Can one of the admins verify this patch?

cc @ninja-

@netty-bot test this please

@njhill yeah, looks like a good change for my main app. Looking forward to it :P

Pushed another commit with small adjustments - noticed that the flags field could have been read from outside the event loop.

@netty-bot test this please

Thanks @normanmaurer, I'm now even more curious what your thoughts are about #9349 :)

@netty-bot test this please

Pushed one more tiny refinement (hopefully last!), sorry for the churn.

@netty-bot test this please

@njhill @carl-mastrangelo after thinking a bit more about this I am not sure if this is really a good idea.

The problem is that this may result in a lot of wakeups even if we are not interested in data (if I am not wrong). Each time new data is received for a connection will have us wakeup just to do nothing. This sounds like it may be very bad in some setups, especially if you have huge receive buffers. Or I am missing anything ?

This of course is only bad if the user stops reading for some extended period of time

@normanmaurer My recollection is that epoll doesn't re notify you until you consume all the data on the FD (which I could be wrong about).

this is hinted by the man page in the Q&A section 9: http://man7.org/linux/man-pages/man7/epoll.7.html

@carl-mastrangelo if this is the case ( I thought it will also wakeup for new data, just not until new arrived) we have a problem here https://github.com/netty/netty/blob/4.1/transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoop.java#L389 as we never read the eventfd value and so would not get another wakup, right ?

@carl-mastrangelo @njhill at least for eventfd it seems my understanding holds.

    public void testET() throws Throwable {
        final FileDescriptor epoll = Native.newEpollCreate();
        final FileDescriptor eventFd = Native.newEventFd();
        final FileDescriptor timerFd = Native.newTimerFd();
        final EpollEventArray array = new EpollEventArray(1024);
        try {
            Native.epollCtlAdd(epoll.intValue(), eventFd.intValue(), Native.EPOLLIN | Native.EPOLLET);
            final AtomicReference<Throwable> causeRef = new AtomicReference<Throwable>();
            final AtomicInteger integer = new AtomicInteger();
            final Thread t = new Thread(new Runnable() {
                @Override
                public void run() {
                    try {
                        for (int i = 0; i < 2; i++) {
                            int ready = Native.epollWait(epoll, array, timerFd, -1, -1);
                            assertEquals(1, ready);
                            assertEquals(eventFd.intValue(), array.fd(0));
                            integer.incrementAndGet();
                        }
                    } catch (IOException e) {
                        causeRef.set(e);
                    }
                }
            });
            t.start();
            Native.eventFdWrite(eventFd.intValue(), 1);
            while (integer.get() != 1) {
                Thread.sleep(100);
            }
            // Ensure there is no other wakeup
            Thread.sleep(1000);
            assertEquals(1, integer.get());
            Native.eventFdWrite(eventFd.intValue(), 1);
            t.join();
            Throwable cause = causeRef.get();
            if (cause != null) {
                throw cause;
            }
            assertEquals(2, integer.get());
        } finally {
            array.free();
            epoll.close();
            eventFd.close();
            timerFd.close();
        }
    }

For sockets it may be different ?

@normanmaurer I tried too, and noticed the same thing. I think you are right, this would cause too much churn.

However, I think reverting the code in clearEpollIn0() might make this work. As long as the unsafe remembers if there is data, and the channel is not in auto read mode, I think the fd can remove its interest in EPOLLIN.

@carl-mastrangelo i will also retry my test with a socket fd to see if there is any difference

@normanmaurer you make a good point :) I had assumed the same as @carl-mastrangelo but after more research it's funny how confusing/misleading and often contradictory the various explanations are on this particular aspect of the behaviour (including the manpages). And it sounds possible that it differs between fd types and/or between the in and out cases. So I agree a socket fd test would be the best next step (hoping that eventfd is special...), I will aim to try too if you haven't already by the time I wake up!

If it turns out to be the case for sockets too then it probably rules out the hoped-for simplification of eliminating all epoll_ctl calls, but I think some could still be avoided - specifically not setting EPOLLIN where it's done currently and instead waiting until the processing is complete, before calling epoll_wait again ... i.e. there's no point in toggling from unset->set->unset while the EL is still active.

I realized the change wasn't even behaving as intended due to an incorrectly inverted check, tests are still fine with that fixed (pushed update), but @normanmaurer it does seem that you're right re new data constituting an edge in the socket case too. I will try to rework it per prior comment.

@njhill let me know once I should have a look again.

@normanmaurer see #9397. We could probably close this one.

Ok lets do it.