Deadlock in server transport with multiple ports

[dapengzhang0]

ServerImpl.start() calls NettyServer.start() while holding ServerImpl.lock. NettyServer.start() awaits a submitted runnable in eventloop. However, this pending runnable may never be executed because the eventloop might be executing some other task, , like ServerListenerImpl.transportCreated(), that is trying to acquire ServerImpl.lock causing a deadlock.

This is a deadlock for multiple-port server transport usecase with the same deadlock mechanism as #6601.

[YifeiZhuang]

Providing more details:
NettyServer.start() holds the lock and will bind the port and await() the bind() asynchronously in the ServerImpl for-loop iterating through each NettyServer.start(). In the critical region, each nettyserver/channel that already bound might be accepting connection already and running channel initialization tasks that is trying to acquire ServerImpl lock in the child eventloop, thus potential deadlock. Question is,

1. Will the ChannelFuture.awaitUninterruptibly() in the parent eventloop have to wait for the channel initialization handler in child eventloop or not? If not, it looks that is no race condition for multiple transport scenario. It may depend on specific implementation of the eventloopgroup. @ejona86 might help confirm.
2. In any case, netty server supports multiple ports, so it is probably cleaner if we make grpc server have one nettyserver only and delegate multiple port heavy lifting to its multiple port support:

You can now call the bind() method as many times as you want (with different bind addresses.)

#6614 attempted a fix w/o moving multi-port to nettyserver but concluded to move forward with multi-port netty.

[dapengzhang0]

2. If so, then ChannelFuture.awaitUninterruptibly() and ServerListenerImpl.transportCreated() race condition exists. It looks like a sub-optimal mitigation, from netty best practice, is to add configure connection timeout to break the deadlock after timeout.

Connection timeout does not help if bind() is not even started. The task for bind() of a new transport may be awaiting the task of transportCreated() of the old transport to finish in the event loop.

It looks so, sorry for the confusion.

[ejona86]

Will the ChannelFuture.awaitUninterruptibly() in the parent eventloop have to wait for the channel initialization handler in child eventloop or not?

"Parent eventloop" doesn't exist here. That is simply "whatever thread the user called start() on."

The await is necessary since Server.start() guarantees that the port will have been bound to before returning (it throws an error if unable).

The problem was that if the await() does not happen in the eventloop, then it supposed not to cause deadlock.

In any case, netty server supports multiple ports, so it is probably cleaner if we make grpc server have one nettyserver only and delegate multiple port heavy lifting to its multiple port support:

Note that that is not a one line change "just call bind multiple times." You can start multiple binds in parallel and then await() for them all at the end, but if any one errors then all the others have to be closed and cleaned up. Also, binding multiple times produces multiple Netty Channels, so any place in NettyServer that references channel will need to iterate over a list of channels.

Yup, and also loop over to in the nettyserver shutdown path as well if we create multiple port on the bootstrap.

[YifeiZhuang]

Problem

Set some context to make sure we understand it correctly:
NettyServer has two distinct sets of channels, corresponding to two eventloopgroups (bosseventloopgroup and workereventloopgroup), the first set is ServerChannels that are listening on the bound local port, the second set of channels are for the accepted connections to handle client requests.

bind() is executed on the ServerChannels on its eventloop, and listener.transportCreated() is called on the second set of channels by its eventloop (executed from ChannelInitializer handler). Usually these eventloops are different but may be the same.
Our problem is ServerImpl.start() runs in a synchronous path holding the lock and blocking on the eventloop, and the eventloop might be executing other tasks (listener.transportCreated()) that is waiting the lock, thus deadlock. This happens in the case when those eventloops happen to be the same one, and there are multiple ports. (Because bind() happens inside a eventloop task and is serialized with the execution of ChannelInitializer handler, so this deadlock won’t happen for a single port case.)

Server deadlock can be reproduced when we force the eventloop to be the same, use multiple ports, and prolong the ServerImpl start between two nettyServer.start(), e.g.

ChannelFuture future = b.bind(address);
Thread.sleep(8000); // extend bind() execution to test deadlock
future.awaitUninterruptibly();

The server will hang there, with only one of the port successfully bind, and client won’t be able to establish connection, error:

Nov 23, 2020 1:02:14 AM com.google.prod.machinelocality.HostnameNoLoas gmiHostname
INFO: Unable to load the hostname from go/google-machine-identity. [CONTEXT ratelimit_period="1 MINUTES" ]
Exception in thread "main" io.grpc.StatusRuntimeException: UNAVAILABLE: Keepalive failed. The connection is likely gone
	at io.grpc.stub.ClientCalls.toStatusRuntimeException(ClientCalls.java:262)
	at io.grpc.stub.ClientCalls.getUnchecked(ClientCalls.java:243)
	at io.grpc.stub.ClientCalls.blockingUnaryCall(ClientCalls.java:156)
	at io.grpc.examples.helloworld.GreeterGrpc$GreeterBlockingStub.sayHello(GreeterGrpc.java:169)
	at com.google.net.grpc.examples.HelloWorldClient.run(HelloWorldClient.java:42)
	at com.google.net.grpc.examples.HelloWorldClient.main(HelloWorldClient.java:31)
201123 01:02:46.817:I 12 [Thread-1] [com.google.common.flogger.backend.google.ShutdownChecker.enterSystemShutdown] Flogger entering shutdown mode; will start logging to stderr.

Solutions

Just moving multi-port to nettyServer layer won’t help with the issue, because bind() multiple ports also happens in the synchronous path while holding the lock. And when first port is successfully bound, clients will be able to send in request and the listener.transportCreate() can still be called to cause the deadlock.

To prevent the deadlock issue, we may have a these options:

1. Move ts.start() out of synchronous path (bb608b6), this can be risky, because not holding the lock at start will allow others to acquire the lock, e.g. shutdown(), which is executing on the eventloop as well and will cause race again.
2. Abort the client connection at channelInitialization handler when we the server has not fully started. Started means binding all the ports successfully. The drawback is that clients won’t be able to communicate when subset of ports are bound while the whole start takes a bit long. But this might be tolerable because server start will fail anyways if we cannot successfully bind() each port.

[ejona86]

The first thing I want to do is to remove support for multiple InternalServers from ServerImpl and have that be handled by NettyServer. (I'm not bothered by multiple InternalServers in general, but multiple during a single server.start() call is a problem. Later, I expect to allow multiple, but with each having a separate start() invocation.) This is so that errors during bind can be properly cleaned up, which is not possible today. (Either we leak a port, or the user can't call server.start() a second time to do a second attempt). It is probably best to make that change first.

(1) wouldn't be that bad and would make sense if ts.start() was able to be aborted. In principal that is a good idea, but we'd need to look at the complexity it brings. If ts.start() returned a future ServerImpl could drop the lock and wait on the future. But ts.shutdown() would need to be operational or we'd server.shutdown() to cancel the Futures.

For (2), it sounds like it would just change this line to be if (channel == null || !channel.isOpen()):

grpc-java/netty/src/main/java/io/grpc/netty/NettyServer.java

Lines 229 to 233 in 3811ef3

	if (channel != null && !channel.isOpen()) {
	// Server already shutdown.
	ch.close();
	return;
	}

That sounds fair, but we'd like to avoid it when we can. I'll note for the single-port case, when that callback fires bind() is already complete.

I'll also note that it looks like this assignment is not thread-safe:

grpc-java/netty/src/main/java/io/grpc/netty/NettyServer.java

Line 270 in 3811ef3

channel = future.channel();

If NettyServer is handling the multiple ports, then we can do all the binds as one "atomic" operation on the event loop. This depends on a behavior of Channel that it executes the operation immediately if already on the event loop. We don't want that long-term, but it may be fine for now. Something like:

EventExecutor bossExecutor = bossGroup.next();
...
// Use bossExecutor instead of bossGroup, which will be a single thread
b.group(bossExecutor, workerGroup);
...
Future<List<ChannelFuture>> future = bossExecutor.submit(new Callable<>() {
  List<ChannelFuture> call() {
    List<ChannelFuture> list = ...;
    for (SocketAddress address : addresses) {
      list.add(b.bind(address));
    }
    return list;
  }
});
future.awaitUninterruptibly();
// process errors
...

Let's just start by moving the multi-port logic into NettyServer, though.

[ejona86]

I'll note that in certain advanced fd-passing schemes, it is useful to pass an already-bound server socket fd. In those schemes, the fd already exists and listen() has been called and may have clients already making connections to it. The only thing missing is some code to accept() connections. In those approaches it is best that we don't drop connections when we start. Now, that sort of thing doesn't happen much with Java, but still.

Systemd socket activation is the most clearly defined usage of this, but it is essentially the same principal used for "hot restarts" of servers (e.g., Envoy). It is also useful when spawning subprocesses that communicate over unix domain sockets.

[YifeiZhuang]

#7674 fixed issue.
#7674 bind multiple ports in a single netty server. The creation of the server socket (in bind()) is atomically in an eventloop and happens before when the client can connect to. After that, the netty server start() does not block on the completion of any eventloop tasks. So even if the eventloops collide, which is rare, the deadlock won't happen. Verified with manual reproduce. Closing.