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

mplex: Check for error and shutdown. #1529

Merged
merged 3 commits into from Apr 1, 2020

Conversation

twittner
Copy link
Contributor

Issues #1504 and #1523 reported panics caused by polling the sink of secio::LenPrefixCodec after it had entered its terminal state, i.e. after it had previously encountered an error or was closed. According to the reports this happened only when using mplex as a stream multiplexer. It seems that because mplex always stores and keeps the Waker when polling, a wakeup of any of those wakers will resume the polling even for those cases where the previous poll did not return Poll::Pending but resolved to a value.

To prevent polling after the connection was closed or an error happened we check for those conditions prior to every poll.

Issues libp2p#1504 and libp2p#1523 reported panics caused by polling the sink of
`secio::LenPrefixCodec` after it had entered its terminal state, i.e.
after it had previously encountered an error or was closed. According
to the reports this happened only when using mplex as a stream
multiplexer. It seems that because mplex always stores and keeps the
`Waker` when polling, a wakeup of any of those wakers will resume the
polling even for those cases where the previous poll did not return
`Poll::Pending` but resolved to a value.

To prevent polling after the connection was closed or an error
happened we check for those conditions prior to every poll.
@romanb
Copy link
Contributor

romanb commented Mar 31, 2020

I may be missing something, but given that a Sink is deemed unusable after an error (and operations may panic), shouldn't we set inner.error in poll_outbound, write_substream, flush_substream, shutdown_substream, close and flush_all so that ensure_no_error_no_close can take effect, i.e. whenever a Sink operation returns an error?

@twittner
Copy link
Contributor Author

I may be missing something, but given that a Sink is deemed unusable after an error (and operations may panic), shouldn't we set inner.error in [...] so that ensure_no_error_no_close can take effect, i.e. whenever a Sink operation returns an error?

I think this would help ;-)

@tomaka tomaka merged commit 3f12a5c into libp2p:master Apr 1, 2020
@twittner twittner deleted the mplex-check-for-error branch April 1, 2020 12:37
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 this pull request may close these issues.

None yet

3 participants