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

Panic in unwrap (for EndpointInner) #283

Closed
ustulation opened this issue Apr 2, 2019 · 2 comments · Fixed by #285
Closed

Panic in unwrap (for EndpointInner) #283

ustulation opened this issue Apr 2, 2019 · 2 comments · Fixed by #285

Comments

@ustulation
Copy link
Contributor

After this was closed #277 (comment) using the new code now sometimes panics with the following backtrace:

8: core::result::unwrap_failed
            at /rustc/2aa4c46cfdd726e97360c2734835aa3515e8c858/src/libcore/macros.rs:16
  9: <core::result::Result<T, E>>::unwrap
            at /rustc/2aa4c46cfdd726e97360c2734835aa3515e8c858/src/libcore/result.rs:798
 10: quinn::endpoint::EndpointInner::drive_recv
            at /home/spandan/.cargo/git/checkouts/quinn-572d2e5812b43165/856b2f5/quinn/src/endpoint.rs:200
 11: <quinn::endpoint::EndpointDriver as futures::future::Future>::poll
            at /home/spandan/.cargo/git/checkouts/quinn-572d2e5812b43165/856b2f5/quinn/src/endpoint.rs:135
 12: <futures::future::map_err::MapErr<A, F> as futures::future::Future>::poll
            at /home/spandan/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-0.1.25/src/future/map_err.rs:30
 13: <alloc::boxed::Box<F> as futures::future::Future>::poll
            at /home/spandan/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-0.1.25/src/future/mod.rs:113

so that's probably https://github.com/djc/quinn/blob/856b2f570a0e72666568403d4ae7b6e9a8627dc3/quinn/src/endpoint.rs#L202

@ustulation ustulation changed the title Panic in unwrap Panic in unwrap (for EndpointInner) Apr 2, 2019
@ustulation
Copy link
Contributor Author

Not sure if it's related but actively calling close() on the Connection resolves IncomingStreams and any read_to_end stuff on already spawned children streams as expected. It doesn't however immediately resolves the driver. That takes it's own sweet time to exit due to error of protocol-timeout (since there's no connection so I'm expecting it's the keepalives+idle-timeout combination which finally are dictating the timeouts). Can you pls confirm that ?

I'm deducing ^^^ from certain printouts but I'm not exclusively checking if that's what's happening. If you can confirm then it'll save me trying to produce that exact scenario. If the driver indeed doesn't resolve on connection closure then i think that too is not great and should be made to do so ?

@Ralith
Copy link
Collaborator

Ralith commented Apr 2, 2019

Thanks for the backtrace, this is really great testing! The backtrace indicates Result rather than Option, so this will actually have to be the second unwrap, at line 204, indicating that the connection driver was dropped before the connection was fully drained. Normally this leads to an artificial Drained event, but there's no guarantee we'll see that before we try sending anything. The fix is trivial, we'll just need to ignore whether the unbounded_send here succeeds or not.

Not sure if it's related but actively calling close() on the Connection resolves IncomingStreams and any read_to_end stuff on already spawned children streams as expected. It doesn't however immediately resolves the driver.

The intended behavior is that the ConnectionDriver waits for the "draining period" to complete, while all other outstanding futures terminate immediately. This is mediated by the Close timer, and takes 3 PTOs, which are individually not a lot longer than an RTT so it shouldn't take very long. If the driver is staying alive for e.g. 10 seconds (the default idle timeout) after you call close, something is probably wrong. The quinn tests wait for all futures to terminate, so we know this works at least in the happy path.

For background: the draining period exists to ensure packets that were already in-flight from the peer when we started closing will be gracefully dropped, rather than eliciting spurious stateless resets; it also gives us opportunity to retransmit the close packet if necessary. It's explicitly permitted to forego the draining period if you're about to close the socket anyway (so there's no risk of spurious resets).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants