Shutdown race in EpollEventLoop

[carl-mastrangelo]

In 4.1.38, there is an fd race in EpollEventLoop. The loop thread, while trying to shutdown, closes the eventFd. The outside thread that calls shutdownGracefully, tries to wake up the loop to notice that it wants it to shutdown. There is a race where the wakeup() call can write to an eventfd that has been closed already, resulting in the possibility of writing to the wrong fd.

The sequence of events loops like:

1. T1: eventLoop.shutdownGracefully()
2. T1: state = ST_SHUTDOWN
3. T2: reads state == ST_SHUTDOWN
4. T1: Enters EpollEventLoop.wakeup()
5. T2: Exits loop, enters EpollEventLoop.cleanup()
6. T2: Closes eventFd.
7. T1: Wins race to by setting wakenUp to 1
8. T1: Calls Native.eventFdWrite(eventFd.intValue(), 1L);

The easiest way I can see to fix this race is to use a synchronized block in wakeup(), after the thread has won the wakenUp = 1 race:

    @Override
    protected void wakeup(boolean inEventLoop) {
        if (!inEventLoop && WAKEN_UP_UPDATER.getAndIncrement(this) == 0) {
            // write to the evfd which will then wake-up epoll_wait(...)
            synchronized (eventFd) {
                if (!isShutdown()) {
                    Native.eventFdWrite(eventFd.intValue(), 1L);
                }
            }
        }
    }

Additionally, in EpollEventLoop.cleanup, acquire a lock before closing the eventFd, synchronize the close:

            synchronized (eventFd) {
                try {
                    eventFd.close();
                } catch (IOException e) {
                    logger.warn("Failed to close the event fd.", e);
                }
            }

This is kind of unfortunate, but the race makes it possible that another FD is opened after closing the eventFd, and the wakeUp call writes to the wrong FD. The main downside of my proposed patch is probably the risk of deadlocks (because I dont know what locks I hold), as well as some performance penalty.

Ideas on how to not pay this cost are welcome, but I think the danger is enough to merit fixing this.

cc @ejona86 @normanmaurer

[normanmaurer]

Hmm is this only in the last release ? Using a synchronized sounds like not a good idea

[carl-mastrangelo]

Not just in last release, but I noticed it while cleaning up races in gRPC.

Are you open to busy polling in shutdown?

[normanmaurer]

Yes this sounds like a better idea

[ejona86]

Can we continue polling until the event loop successfully sets wakenUp to 1, after which point it closes the fds? If the event loop sets wakenUp to 1, then wakeup()'s getAndSet will become a "noop."

[carl-mastrangelo]

@ejona86 We can't because wakeup may have already won the race, but not yet written to the fd.

I thought about this more over the weekend, and I can't see a way to do with without both an "enter" and "exit" atomic op. The loop doesn't know if there is an in-progress or upcoming write to the fd.

[ejona86]

We can't because wakeup may have already won the race, but not yet written to the fd.

We know when we lose the race as wakeup will be 1. In that case we wait on the eventfd for the event we know is coming and then we "try again." Except we could choose not to set wakeup to 0, so I guess we just leave wakeup as-is and are now safe to close the eventfd.

Note that means that wakeup then becomes "load bearing." Before it was an optimization. Now it is critical and must not use eventfd when wakeup is 1.

[carl-mastrangelo]

losing the race means that wakenUp will be 1, but since the eventfd is in non-blocking mode, we would have to poll it until it returns eagain. I dislike the idea of making close required to read from the fd, so I changed the logic to be a tri-state.