Upstream/ProtocolLib
1
0
Fork 0
mirror of https://github.com/dmulloy2/ProtocolLib.git synced 2024-11-24 03:25:29 +01:00
Code Issues Projects Releases Wiki Activity

A possible fix for a rare but game-breaking deadlock.

Browse Source 
Calling remove() in the main thread will block the main thread, which 
may lead to a deadlock:
    http://pastebin.com/L3SBVKzp

ProtocolLib executes this close() method through a PlayerQuitEvent in 
the main thread, which has implicitly aquired a lock on 
SimplePluginManager (see SimplePluginManager.callEvent(Event)). 
Unfortunately, the remove() method will schedule the removal on one of 
the Netty worker threads if it's called from a different thread, 
blocking until the removal has been confirmed.
 
This is bad enough (Rule #1: Don't block the main thread), but the real 
trouble starts if the same worker thread happens to be handling a server
ping connection when this removal task is scheduled. In that case, it 
may attempt to invoke an asynchronous ServerPingEvent 
(see PacketStatusListener) using SimplePluginManager.callEvent(). But, 
since this has already been locked by the main thread, we end up with a 
deadlock. The main thread is waiting for the worker thread to process 
the task, and the worker thread is waiting for the main thread to 
finish executing PlayerQuitEvent.

TLDR: Concurrenty is hard.
This commit is contained in:
Kristian S. Stangeland 2013-12-24 02:15:22 +01:00
parent 16dd2d5d1b
commit e56c0fec00
2 changed files with 39 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/BootstrapList.java
View File

@ -4,10 +4,12 @@ import java.util.Collection;
import java.util.Iterator; import java.util.Iterator;
import java.util.List; import java.util.List;
import java.util.ListIterator; import java.util.ListIterator;
import java.util.NoSuchElementException; import java.util.concurrent.Callable;
import com.google.common.collect.Lists; import com.google.common.collect.Lists;
import net.minecraft.util.io.netty.channel.Channel;
// Hopefully, CB won't version these as well // Hopefully, CB won't version these as well
import net.minecraft.util.io.netty.channel.ChannelFuture; import net.minecraft.util.io.netty.channel.ChannelFuture;
import net.minecraft.util.io.netty.channel.ChannelHandler; import net.minecraft.util.io.netty.channel.ChannelHandler;
@ -94,11 +96,16 @@ class BootstrapList implements List<Object> {
* @param future - the future. * @param future - the future.
*/ */
protected void unprocessBootstrap(ChannelFuture future) { protected void unprocessBootstrap(ChannelFuture future) {
try { final Channel channel = future.channel();
future.channel().pipeline().remove(handler);
} catch (NoSuchElementException e) { // For thread safety - see ChannelInjector.close()
// Whatever channel.eventLoop().submit(new Callable<Object>() {
} @Override
public Object call() throws Exception {
channel.pipeline().remove(handler);
return null;
}
});
} }
/** /**

33
ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelInjector.java
View File

@ -6,8 +6,8 @@ import java.net.SocketAddress;
import java.util.Collections; import java.util.Collections;
import java.util.List; import java.util.List;
import java.util.Map.Entry; import java.util.Map.Entry;
import java.util.NoSuchElementException;
import java.util.Set; import java.util.Set;
import java.util.concurrent.Callable;
import java.util.concurrent.ConcurrentMap; import java.util.concurrent.ConcurrentMap;
import net.minecraft.util.com.mojang.authlib.GameProfile; import net.minecraft.util.com.mojang.authlib.GameProfile;
@ -510,12 +510,31 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector {
if (injected) { if (injected) {
channelField.revertValue(); channelField.revertValue();
try { // Calling remove() in the main thread will block the main thread, which may lead
originalChannel.pipeline().remove(this); // to a deadlock:
originalChannel.pipeline().remove(protocolEncoder); // http://pastebin.com/L3SBVKzp
} catch (NoSuchElementException e) { //
// Ignore it - the player has logged out // ProtocolLib executes this close() method through a PlayerQuitEvent in the main thread,
} // which has implicitly aquired a lock on SimplePluginManager (see SimplePluginManager.callEvent(Event)).
// Unfortunately, the remove() method will schedule the removal on one of the Netty worker threads if
// it's called from a different thread, blocking until the removal has been confirmed.
//
// This is bad enough (Rule #1: Don't block the main thread), but the real trouble starts if the same
// worker thread happens to be handling a server ping connection when this removal task is scheduled.
// In that case, it may attempt to invoke an asynchronous ServerPingEvent (see PacketStatusListener)
// using SimplePluginManager.callEvent(). But, since this has already been locked by the main thread,
// we end up with a deadlock. The main thread is waiting for the worker thread to process the task, and
// the worker thread is waiting for the main thread to finish executing PlayerQuitEvent.
//
// TLDR: Concurrenty is hard.
originalChannel.eventLoop().submit(new Callable<Object>() {
@Override
public Object call() throws Exception {
originalChannel.pipeline().remove(ChannelInjector.this);
originalChannel.pipeline().remove(protocolEncoder);
return null;
}
});
// Clear cache // Clear cache
factory.invalidate(player); factory.invalidate(player);
} }