Repository: netty/netty
Issue: (Nio|Epoll)EventLoop.pendingTasks should not block when called outside of event loop
Labels: improvement

### Expected behavior

`EventLoop.pendingTasks` should be (reasonably) cheap to invoke so it can be used within observability.

### Actual behavior

We've been experimenting with overriding `EventExecutorChooserFactory` with more superior load balancing algorithms and found out that it might be quite non-trivial to support pending-tasks load metric (pick the least loaded event loop).

After poking around the code, it seems when querying `pendingTasks` of either `NioEventLoop`  or `EpollEventLoop`, the method [blocks if called outside of said event loop](https://github.com/netty/netty/blob/630c82717dd8d822323c9e4a8ad51ea4f8a38f86/transport/src/main/java/io/netty/channel/nio/NioEventLoop.java#L262-L272). Put it this way, it's only cheap to query event loop's pending tasks from within the same event loop. Perhaps I miss some (historical) context for `pendingTask`, but for an uneducated reader, it just defeats the purpose of a method providing visibility into an event loop.

I was hoping to pick up some context on the current behavior of `pendingTasks` and see if we ever want/will to change it.


Issue comments:
@vkostyukov thats a good point... We may want to "replace" the current implementation with an `AtomicInteger` that is updated . WDYT ?
@normanmaurer Yeah, that seems reasonable to me. Even if we won't be able to guarantee certain precision it should be totally fine for the sake of observability (provided we document it).

I'd be happy to contribute a fix for this unless somebody else wants to work on this.
@vkostyukov that would be awesome... Like you already stated we should just make this "as cheap as possible". 
@normanmaurer @Wingfril, CSL's summer intern, want to give this PR a try (she is working on overriding the event loop chooser in Finagle).
Perfect... let her know I am always happy to help 👍

> Am 14.08.2018 um 22:20 schrieb Vladimir Kostyukov <notifications@github.com>:
> 
> @normanmaurer @Wingfril, CSL's summer intern, want to give this PR a try (she is working on overriding experimenting with event loop chooser in Finagle).
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.

I'm not really sure why we even need to do that? Can't we just get the super.size() itself without worrying if it is in the event loop? It seems `MpscQueue` is no longer in the code, and so live locking shouldn't be an issue anymore? `SingleThreadEventExecutor` uses a `LinkedBlockingQueue` which has a size variable which is an `atomicInteger` already?

Woops
@Wingfril it's still in the code, see `io.netty.channel.nio.NioEventLoop#newTaskQueue`, `io.netty.channel.epoll.EpollEventLoop#newTaskQueue` and `io.netty.channel.kqueue.KQueueEventLoop#newTaskQueue`
@johnou actually I think @Wingfril is right, the original issue from which the run-in-eventloop workaround stemmed was based on a livelock issue (https://github.com/netty/netty/issues/3675, https://github.com/netty/netty/issues/5297) encountered with the old netty-modified `MpscLinkedQueue` implementation used at the time

This was since replaced by the shaded jctools impl in https://github.com/netty/netty/pull/5051 (by you I see!), and there's no reason to suspect its `size()` method suffers from the same problem.

So I would propose just to undo the original run-in-eventloop workaround and instead call `size()` on the queue directly, @normanmaurer do you agree?
@njhill it may have been updated but it's still a single consumer queue.

@nitsanw is it safe to invoke MpscQueue.size() from multiple threads? Given the name, single consumer I would lean towards no, does it still risk producing a livelock?
Yes

On Thu, Aug 16, 2018, 13:44 Johno Crawford <notifications@github.com> wrote:

> @nitsanw <https://github.com/nitsanw> is it safe to invoke
> MpscQueue.size() from multiple threads?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/netty/netty/issues/8196#issuecomment-413517653>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/ACkWLzc1jkQqpmOi4nLxrP9LSfwX09YQks5uRVsWgaJpZM4V802C>
> .
>

Then please just remove the code 👍🔥

> Am 16.08.2018 um 16:27 schrieb Nitsan Wakart <notifications@github.com>:
> 
> Yes
> 
> On Thu, Aug 16, 2018, 13:44 Johno Crawford <notifications@github.com> wrote:
> 
> > @nitsanw <https://github.com/nitsanw> is it safe to invoke
> > MpscQueue.size() from multiple threads?
> >
> > —
> > You are receiving this because you were mentioned.
> > Reply to this email directly, view it on GitHub
> > <https://github.com/netty/netty/issues/8196#issuecomment-413517653>, or mute
> > the thread
> > <https://github.com/notifications/unsubscribe-auth/ACkWLzc1jkQqpmOi4nLxrP9LSfwX09YQks5uRVsWgaJpZM4V802C>
> > .
> >
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.

@nitsanw was the yes for safe or risk producing a livelock? :)
@johnou I'm pretty sure the livelock issue wasn't inherently related to the single-consumerness. Looking at the history it appeared to be more likely linked to the custom modifications made (e.g. possibly `RecyclableMpscLinkedQueueNode`).

Of course the reported size may not be "precise" due to the lazy semantics but should hopefully be "safe".
You asked if it was safe, right?

> On 16 Aug 2018, at 16:58, Johno Crawford <notifications@github.com> wrote:
> 
> @nitsanw was the yes for safe or risk producing a livelock? :)
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.

@nitsanw and also asked if it was unsafe and posed risks ie. livelock, but given your comment now I will assume it's safe to invoke size() from multiple threads, thanks!
@johnou I apologize if I sounded impatient :-) I was reading your question from the GitHub email. Here's what I got:
> @nitsanw is it safe to invoke MpscQueue.size() from multiple threads?

It appears that further edits do not get sent as further e-mails.

So, yes, it's safe to call size on any queue in jctools from any thread, no issue. Furthermore, current queue being used is either the MpscChunkedArrayQueue or MpscUnboundedArrayQueue:
https://github.com/netty/netty/blob/06f3574e46e0623a45a93d36dbe7aa3d7455a995/common/src/main/java/io/netty/util/internal/PlatformDependent.java#L823

Both of which have a very fast `size()` method.

As a side note, I'm not sure why in the non-unsafe case you do not use MpscChunkedAtomicArrayQueue:
https://github.com/netty/netty/blob/06f3574e46e0623a45a93d36dbe7aa3d7455a995/common/src/main/java/io/netty/util/internal/PlatformDependent.java#L859

Thanks
@nitsanw maybe consider doing a PR ? :)
@nitsanw all good, cheers for the follow-up!
@vkostyukov fixes via https://github.com/netty/netty/pull/8197 by @Wingfril :)
@vkostyukov btw if there is anything that makes sense to contribute back to netty in terms of a custom `EventExecutorChooserFactory` I would be more then happy to review it :)
@normanmaurer We've been getting pretty mixed results so far (@Wingfril can probably speak more about it) as we weren't really able to employ the `pendingTask` method.
@Wingfril I would very interested to hear more.
@normanmaurer 
Heyo. 
So we tried several different load metrics to try and apply power of 2 choices (and also a heap when using number of channels as a load metric), such as keeping track of the amount of channels in each eventloop, looking at the pending tasks per eventloop, and also I modified a local copy of netty to look at the awake time/alive time of the eventloop. 

In staging and in simulations, none of the metrics that we tried had improved overall latency or throughput. In fact, I don't think there was even a decrease in standard deviations between stats of the netty default and 2 of the load metrics. When we looked at the number of channels per eventloop, we were able to make the channels more evenly spread out to the eventloops, but again, this did not translate to performance improvements. 
@Wingfril interesting.. Let me know if you find anything interesting in the future. 

Pull request review comments:
I think all these changes should go into `EpollEventLoop` / `NioEventLoop` / `KQueueEventLoop` and the `SingleThreadEventExecutor` should not be changed. This is mainly because otherwise it may produce unexpected effects for people that extend the `SingleThreadEventExecutor`. 
This should be final btw.

Patch that resolved the issue:
diff --git a/common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java b/common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
index bb51d8f3962..be5814f5955 100644
--- a/common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
+++ b/common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
@@ -304,7 +304,7 @@ protected boolean hasTasks() {
      * Return the number of tasks that are pending for processing.
      *
      * <strong>Be aware that this operation may be expensive as it depends on the internal implementation of the
-     * SingleThreadEventExecutor. So use it was care!</strong>
+     * SingleThreadEventExecutor. So use it with care!</strong>
      */
     public int pendingTasks() {
         return taskQueue.size();
diff --git a/transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoop.java b/transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoop.java
index 68e9c125a18..c90a2ca1069 100644
--- a/transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoop.java
+++ b/transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoop.java
@@ -73,12 +73,6 @@ public int get() throws Exception {
             return epollWaitNow();
         }
     };
-    private final Callable<Integer> pendingTasksCallable = new Callable<Integer>() {
-        @Override
-        public Integer call() throws Exception {
-            return EpollEventLoop.super.pendingTasks();
-        }
-    };
     private volatile int wakenUp;
     private volatile int ioRatio = 50;
 
@@ -215,17 +209,6 @@ protected Queue<Runnable> newTaskQueue(int maxPendingTasks) {
                                                     : PlatformDependent.<Runnable>newMpscQueue(maxPendingTasks);
     }
 
-    @Override
-    public int pendingTasks() {
-        // As we use a MpscQueue we need to ensure pendingTasks() is only executed from within the EventLoop as
-        // otherwise we may see unexpected behavior (as size() is only allowed to be called by a single consumer).
-        // See https://github.com/netty/netty/issues/5297
-        if (inEventLoop()) {
-            return super.pendingTasks();
-        } else {
-            return submit(pendingTasksCallable).syncUninterruptibly().getNow();
-        }
-    }
     /**
      * Returns the percentage of the desired amount of time spent for I/O in the event loop.
      */
diff --git a/transport-native-kqueue/src/main/java/io/netty/channel/kqueue/KQueueEventLoop.java b/transport-native-kqueue/src/main/java/io/netty/channel/kqueue/KQueueEventLoop.java
index 5af59ba912c..64a09c97d1b 100644
--- a/transport-native-kqueue/src/main/java/io/netty/channel/kqueue/KQueueEventLoop.java
+++ b/transport-native-kqueue/src/main/java/io/netty/channel/kqueue/KQueueEventLoop.java
@@ -66,12 +66,6 @@ public int get() throws Exception {
             return kqueueWaitNow();
         }
     };
-    private final Callable<Integer> pendingTasksCallable = new Callable<Integer>() {
-        @Override
-        public Integer call() throws Exception {
-            return KQueueEventLoop.super.pendingTasks();
-        }
-    };
 
     private volatile int wakenUp;
     private volatile int ioRatio = 50;
@@ -301,14 +295,6 @@ protected Queue<Runnable> newTaskQueue(int maxPendingTasks) {
                                                     : PlatformDependent.<Runnable>newMpscQueue(maxPendingTasks);
     }
 
-    @Override
-    public int pendingTasks() {
-        // As we use a MpscQueue we need to ensure pendingTasks() is only executed from within the EventLoop as
-        // otherwise we may see unexpected behavior (as size() is only allowed to be called by a single consumer).
-        // See https://github.com/netty/netty/issues/5297
-        return inEventLoop() ? super.pendingTasks() : submit(pendingTasksCallable).syncUninterruptibly().getNow();
-    }
-
     /**
      * Returns the percentage of the desired amount of time spent for I/O in the event loop.
      */
diff --git a/transport/src/main/java/io/netty/channel/nio/NioEventLoop.java b/transport/src/main/java/io/netty/channel/nio/NioEventLoop.java
index 2c197775a92..e36e24a2797 100644
--- a/transport/src/main/java/io/netty/channel/nio/NioEventLoop.java
+++ b/transport/src/main/java/io/netty/channel/nio/NioEventLoop.java
@@ -71,12 +71,6 @@ public int get() throws Exception {
             return selectNow();
         }
     };
-    private final Callable<Integer> pendingTasksCallable = new Callable<Integer>() {
-        @Override
-        public Integer call() throws Exception {
-            return NioEventLoop.super.pendingTasks();
-        }
-    };
 
     // Workaround for JDK NIO bug.
     //
@@ -259,18 +253,6 @@ protected Queue<Runnable> newTaskQueue(int maxPendingTasks) {
                                                     : PlatformDependent.<Runnable>newMpscQueue(maxPendingTasks);
     }
 
-    @Override
-    public int pendingTasks() {
-        // As we use a MpscQueue we need to ensure pendingTasks() is only executed from within the EventLoop as
-        // otherwise we may see unexpected behavior (as size() is only allowed to be called by a single consumer).
-        // See https://github.com/netty/netty/issues/5297
-        if (inEventLoop()) {
-            return super.pendingTasks();
-        } else {
-            return submit(pendingTasksCallable).syncUninterruptibly().getNow();
-        }
-    }
-
     /**
      * Registers an arbitrary {@link SelectableChannel}, not necessarily created by Netty, to the {@link Selector}
      * of this event loop.  Once the specified {@link SelectableChannel} is registered, the specified {@code task} will


Test patch: