DefaultPromise StackOverflowError protection #5302
Conversation
|
@normanmaurer @ejona86 - PTAL |
Scottmitch
force-pushed
the
default_promise_stackoverflow
branch
2 times, most recently
from
9d0ef1b to
ed61ec4
Compare
| @@ -420,18 +432,88 @@ private void notifyListeners() { | |||
| } | |||
| EventExecutor executor = executor(); | |||
| if (executor.inEventLoop()) { | |||
There was a problem hiding this comment.
Why don't you do the stack protection here? If the stack depth is too great, schedule it on the executor. Is there concern that the executor may run the OneTimeTask immediately (not enqueuing it)?
There was a problem hiding this comment.
Is there concern that the executor may run the OneTimeTask immediately (not enqueuing it)?
Yes. For example ImmediateExecutor (and executors which behave in a similar fashion) would not work with the "run a task on the executor" approach.
There was a problem hiding this comment.
Should we really care much about the ImmediateExecutor case? Within Netty, ImmediateExecutor is only used in a minor way in SslHandler, and I have difficulty thinking it is used frequently by user code for Promises. It seems if ImmediateExecutor causes stack depth problems (which is can on its own), then maybe it should be fixed in its own way and not worry about it here.
There was a problem hiding this comment.
The transport focused executors (backed by SingleThreadEventExecutor) could also get into trouble. For example If there is a promise listener chain:
A (operation completes calls B.success(...))
B (operation completes calls N.success(...))
N (operation completes calls (N+1).success(...))
If N is sufficiently large, or the available stack space is sufficiently small relative to N, we may cause a SOE. We currently have some vulnerability to this, but the stack growth due to promise listener notification is at least capped to MAX_LISTENER_STACK_DEPTH.
We may be able to do a similar type of fix in ImmediateExecutor, 'EmbeddedEventLoop', 'ImmediateEventExecutor, transport executors, etc... in Netty ... but there are also other implementations ofExecutorthat we have no control over (things similar to guava'sDirectExecutor`). Would you be interested in submitting a PR for this?
There was a problem hiding this comment.
The transport focused executors (backed by SingleThreadEventExecutor) could also get into trouble.
I don't believe that is true. SingleThreadEventExecutor uses a queue. Same with EmbeddedEventLoop. Any executor that uses a queue should be fine with the approach I mentioned (still use ThreadLocal, but when too large schedule a Runnable).
We may be able to do a similar type of fix in ImmediateExecutor, 'EmbeddedEventLoop', 'ImmediateEventExecutor, transport executors, etc... in Netty ... but there are also other implementations ofExecutorthat we have no control over (things similar to guava'sDirectExecutor`).
I only believe ImmediateEventExecutor and directExecutor are impacted. You're talking about working around the stack depth problem for every user of an Executor (which there are many more of than Executor implementations). And it seems using ImmediateEventExecutor would be rare for Promise given Netty's synchronization model.
It may be a different conversation if we were talking about ListenableFuture in Guava, because directExecutor is used very frequently there since each callback provides its own independent Executor. And nothing is able to optimize for inEventLoop() which causes greater use of directExecutor. And you can't call ctx.executor() in code outside of Netty, so if you need an executor it has to be injected.
There was a problem hiding this comment.
I don't believe that is true.
SingleThreadEventExecutoruses a queue.
Yes you are right. I agree. Items will be queued and run later.
|
I've got a comment about the chosen approach, but the code looks sound. LGTM |
|
@ejona86 - thanks for the review. @normanmaurer - I will pull in now to be sure it gets into the next release ... let me know if you find any issues and I can fix/cleanup. |
Motivation: f2ed3e6 removed the previous mechanism for StackOverflowError because it didn't work in all cases (i.e. ImmediateExecutor). However if a chain of listeners which complete other promises is formed there is still a possibility of a StackOverflowError. Modifications: - Use a ThreadLocal to save any DefaultPromises which could not be notified due to the stack being too large. After the first DefaultPromise on the stack completes notification this ThreadLocal should be used to notify any DefaultPromises which have not yet been notified. Result: DefaultPromise has StackOverflowError protection that works with all EventExecutor types.
Scottmitch
force-pushed
the
default_promise_stackoverflow
branch
from
ed61ec4 to
2ba5132
Compare
Motivation:
f2ed3e6 removed the previous mechanism for StackOverflowError because it didn't work in all cases (i.e. ImmediateExecutor). However if a chain of listeners which complete other promises is formed there is still a possibility of a StackOverflowError.
Modifications:
Result:
DefaultPromise has StackOverflowError protection that works with all EventExecutor types.