Skip to content
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

Never send requests after transport service is stopped #7862

Merged
merged 1 commit into from Sep 25, 2014

Conversation

Projects
None yet
3 participants
@s1monw
Copy link
Contributor

commented Sep 24, 2014

With local transport or any transport that doesn't necessarily send
notification if connections are closed we might miss a node
disconnection and the request handler hangs forever / until the timeout
kicks in. This window only exists during shutdown and is likely
unproblematic in practice but tests might run into this problem when
local transport is used.

@@ -191,11 +197,14 @@ public void removeConnectionListener(TransportConnectionListener listener) {
final long requestId = newRequestId();
TimeoutHandler timeoutHandler = null;
try {
clientHandlers.put(requestId, new RequestHolder<>(handler, node, action, timeoutHandler));
if (started.get() == false) {

This comment has been minimized.

Copy link
@bleskes

bleskes Sep 25, 2014

Member

This can cause double exception handling, one on the caller thread and one on the handler (if the transport is closed between the two lines). I think we should rely on the clientHandels (concurrent map) and rather then throw an exception do the same logic we have in the finally clause in doStop - removed the holder from the clientHandlers, if that works out, call the handler with an exception. Makes sense?

@bleskes

This comment has been minimized.

Copy link
Member

commented Sep 25, 2014

I think this good to do, I left one comment w.r.t to potentially double error handling.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2014

@bleskes I think you missed the handling in the catch block...I pushed a new commit with some documetnation for that

@bleskes

This comment has been minimized.

Copy link
Member

commented Sep 25, 2014

@s1monw yes, missed it - was folded away. LGTM

[TRANSPORT] never send requests after transport service is stopped
With local transport or any transport that doesn't necessarily send
notification if connections are closed we might miss a node
disconnection and the request handler hangs forever / until the timeout
kicks in. This window only exists during shutdown and is likely
unproblematic in practice but tests might run into this problem when
local transport is used.

@s1monw s1monw force-pushed the s1monw:mark_transport_service_stopped branch to a90d7b1 Sep 25, 2014

@s1monw s1monw merged commit a90d7b1 into elastic:master Sep 25, 2014

@s1monw s1monw deleted the s1monw:mark_transport_service_stopped branch Sep 25, 2014

@s1monw s1monw removed the review label Sep 25, 2014

@clintongormley clintongormley changed the title [TRANSPORT] never send requests after transport service is stopped Internal: Never send requests after transport service is stopped Sep 26, 2014

@clintongormley clintongormley changed the title Internal: Never send requests after transport service is stopped Never send requests after transport service is stopped Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.