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

src/connection: Return error from next_frame Future early #137

Closed
wants to merge 1 commit into from

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Aug 1, 2022

The frame future might be ready with an Error from the underlying
socket (i.e. here libp2p-websocket). Though given that the result of the
control_command Future is handled first, on_control_command is called
despite frame having returned an Error. on_control_command itself may try
to write to the underlying socket, which will panic given that the socket
returned an error earlier via the frame Future.

Patch to validate suspicion in
libp2p/rust-libp2p#2598.

Note that I am not planning to merge this as is. This needs more refactoring in order to make it into master.

I would very much appreciate help testing this patch. You can add the override below to your Cargo.toml to compile your binary with the patch:

[patch.crates-io]
yamux = { git = 'https://github.com/mxinden/yamux', branch = "next-frame-error" }

Please double check your Cargo.lock to make sure the patched version is being used to compile your binary.

> The `frame` future might be _ready_ with an `Error` from the underlying
socket (i.e. here `libp2p-websocket`). Though given that the result of the
`control_command` `Future` is handled first, `on_control_command` is called
despite `frame` having returned an `Error`. `on_control_command` itself may try
to write to the underlying socket, which will panic given that the socket
returned an error earlier via the `frame` `Future`.

Patch to validate suspicion in
libp2p/rust-libp2p#2598.
@jasl
Copy link

jasl commented Aug 1, 2022

the patch should be

[patch.crates-io]
yamux = { git = 'https://github.com/mxinden/yamux', branch = "next-frame-error" }

jasl added a commit to Phala-Network/khala-parachain that referenced this pull request Aug 1, 2022
@mxinden
Copy link
Member Author

mxinden commented Aug 1, 2022

Correct. Thanks @jasl. Updated above.

@mxinden
Copy link
Member Author

mxinden commented Aug 4, 2022

Closing here in favor of #138.

@mxinden mxinden closed this Aug 4, 2022
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.

2 participants