New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow ActionListener to be called on the network thread #10573
Conversation
return null; | ||
} | ||
return sync.getValue(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning null
feels wrong to me as then there is no difference between tryGet if the computation is not done and if it actually returns null
. Maybe we need different semantics such as throwing an exception instead of returning null
is sync.isDone() returns false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I considered a couple of approaches:
- The one above (overloading the
null
value to indicate not done). - Having a special case for doing get with timeout set to 0 where we don't assert on a networking thread.
- Having a method like you suggest called something like
getExpectingDone
which is expected to be call only after the future is guaranteed to be done. If feeltryGet
sounds like something very light where the expectation is that is not done (i.e., a syntactic sugar for if (isDone) get() ).
I don't like any of the above much, but I had a slight preference to the first. If you like the second better, I can do that. Of course, new ideas are welcome :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had not though about option 2 but I really like the fact that it does not require to add a new API.
But if this argument does not resonate to you, feel free to push the change as-in, I don't have too strong feelings about it.
To protect ourselves against running blocking operations on a network thread we have added an assertion that triggers that verifies that the thread calling a BaseFuture.get() is not a networking one. While this assert is good, it wrongly triggers when the get() is called in order to pass it's result to a listener of AbstractListenableActionFuture who is marked to run on the same thread as the callee. At that point, we know that the operation has been completed and the get() call will not block. To solve this, we add a tryGet() option which is guaranteed not to block. Relates to elastic#10402
1056ec8
to
1470cfe
Compare
@jpountz I applied your feedback and rebased. Can you have another look? |
LGTM |
To protect ourselves against running blocking operations on a network thread we have added an assertion that triggers that verifies that the thread calling a BaseFuture.get() is not a networking one. While this assert is good, it wrongly triggers when the get() is called in order to pass it's result to a listener of AbstractListenableActionFuture who is marked to run on the same thread as the callee. At that point, we know that the operation has been completed and the get() call will not block. To solve this, we change the assertion to ignore a get with a timeout of 0 and use that AbstractListenableActionFuture Relates to #10402 Closes #10573
To protect ourselves against running blocking operations on a network thread we have added an assertion that triggers that verifies that the thread calling a BaseFuture.get() is not a networking one. While this assert is good, it wrongly triggers when the get() is called in order to pass it's result to a listener of AbstractListenableActionFuture who is marked to run on the same thread as the callee. At that point, we know that the operation has been completed and the get() call will not block. To solve this, we change the assertion to ignore a get with a timeout of 0 and use that AbstractListenableActionFuture Relates to #10402 Closes #10573
To protect ourselves against running blocking operations on a network thread we have added an assertion that triggers that verifies that the thread calling a BaseFuture.get() is not a networking one. While this assert is good, it wrongly triggers when the get() is called in order to pass it's result to a listener of AbstractListenableActionFuture who is marked to run on the same thread as the callee. At that point, we know that the operation has been completed and the get() call will not block.
To solve this, we change the assertion to ignore a get with a timeout of 0 and use that AbstractListenableActionFuture
Relates to #10402