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.

Sign up for GitHub

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

Jump to bottom

websocket/stream: Fix unexpected EOF on Poll::Pending state poisoning #327

Merged
merged 12 commits into from
Feb 19, 2025
Merged

websocket/stream: Fix unexpected EOF on Poll::Pending state poisoning #327

merged 12 commits into from
Feb 19, 2025

Conversation

lexnv
Copy link
Collaborator

@lexnv lexnv commented Feb 18, 2025

This PR updates the following dependencies:

  • tokio tungstenite
  • URL crate to facilitate the new tungstenite version

The tokio tungstenite update changed the API of webscoket messages, which motivated the following changes:

  • Whenpoll_ready or poll_flush returned Poll::Pending, the websocket state would forever remain poisoned.
    • This has the side effect of returning UnexpectedEof on consecutive calls, which led to the connection being terminated
  • The state machine of the websocket is simplified to just 3 states
  • Writer buffer and pointer are merged under one BytesMut instance
  • poll_read implementation is simplified as well to make the code more readable

lexnv added 12 commits February 17, 2025 20:13
@lexnv
cargo: Update tokio_tungstenite to latest version and URL as well
1edb77a 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
cargo: Update cargo lock
546de11 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
websocket/stream: Update code to function with latest tungstenite
429bca7 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
websocket: Do not call start_send without proper poll_ready before hand
0fb83be 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
websocket/stream: Remove ReadyToFlush state that caused unnecessary m…
be8c8fd 
…em allocs

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
websocket/stream: Refactor flush pending branch
131b6cf 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
websocket/stream: Replace write buffer with bytesmut
f4e661b 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
websocket/stream: Avoid extra allocation by freezing bytes
af0d48e 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
websocket/stream: Remove write_ptr
0878a26 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
websocket/stream: Simplify poll read state
fded560 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
websocket/stream: Fix unexpected EOF on Poll::Pending state poisoning
be19cc5 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
Merge remote-tracking branch 'origin/master' into lexnv/websocket-update
25f48f5 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv self-assigned this Feb 18, 2025
dmitry-markin
dmitry-markin approved these changes Feb 19, 2025
View reviewed changes
Copy link
Collaborator

@dmitry-markin dmitry-markin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch with a poisoned state being left!

@lexnv lexnv merged commit 6e40fbb into master Feb 19, 2025
8 checks passed
@lexnv lexnv deleted the lexnv/websocket-update branch February 19, 2025 15:58
@lexnv lexnv mentioned this pull request Feb 19, 2025
Merged
lexnv added a commit that referenced this pull request Feb 20, 2025
@lexnv
chore: Release litep2p 0.9.1 (#328)
2a49fd0 
## [0.9.1] - 2025-01-19

This release enhances compatibility between litep2p and libp2p by
backporting the latest Yamux updates. Additionally, it includes various
improvements and fixes to boost the stability and performance of the
WebSocket stream and the multistream-select protocol.

### Changed

- yamux: Switch to upstream implementation while keeping the controller
API ([#320](#320))
- req-resp: Replace SubstreamSet with FuturesStream
([#321](#321))
- cargo: Bring up to date multiple dependencies
([#324](#324))
- build(deps): bump hickory-proto from 0.24.1 to 0.24.3
([#323](#323))
- build(deps): bump openssl from 0.10.66 to 0.10.70
([#322](#322))

### Fixed

- websocket/stream: Fix unexpected EOF on `Poll::Pending` state
poisoning ([#327](#327))
- websocket/stream: Avoid memory allocations on flushing
([#325](#325))
- multistream-select: Enforce `io::error` instead of empty protocols
([#318](#318))
- multistream: Do not wait for negotiation in poll_close
([#319](#319))

cc @paritytech/networking

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv mentioned this pull request Feb 20, 2025
Merged
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this pull request Feb 20, 2025
@lexnv
net/litep2p: Bring the latest compatibility fixes via v0.9.1 (#7640)
42e9de7 
This PR updates litep2p to version 0.9.1. The yamux config is entirely
removed to mirror the libp2p yamux upstream version.
While at it, I had to bump indexmap and URL as well. 


## [0.9.1] - 2025-01-19

This release enhances compatibility between litep2p and libp2p by using
the latest Yamux upstream version. Additionally, it includes various
improvements and fixes to boost the stability and performance of the
WebSocket stream and the multistream-select protocol.

### Changed

- yamux: Switch to upstream implementation while keeping the controller
API ([#320](paritytech/litep2p#320))
- req-resp: Replace SubstreamSet with FuturesStream
([#321](paritytech/litep2p#321))
- cargo: Bring up to date multiple dependencies
([#324](paritytech/litep2p#324))
- build(deps): bump hickory-proto from 0.24.1 to 0.24.3
([#323](paritytech/litep2p#323))
- build(deps): bump openssl from 0.10.66 to 0.10.70
([#322](paritytech/litep2p#322))

### Fixed

- websocket/stream: Fix unexpected EOF on `Poll::Pending` state
poisoning ([#327](paritytech/litep2p#327))
- websocket/stream: Avoid memory allocations on flushing
([#325](paritytech/litep2p#325))
- multistream-select: Enforce `io::error` instead of empty protocols
([#318](paritytech/litep2p#318))
- multistream: Do not wait for negotiation in poll_close
([#319](paritytech/litep2p#319))

cc @paritytech/networking

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
github-actions bot pushed a commit to paritytech/polkadot-sdk that referenced this pull request Feb 20, 2025
@lexnv @github-actions
net/litep2p: Bring the latest compatibility fixes via v0.9.1 (#7640)
76e9c54 
This PR updates litep2p to version 0.9.1. The yamux config is entirely
removed to mirror the libp2p yamux upstream version.
While at it, I had to bump indexmap and URL as well.

## [0.9.1] - 2025-01-19

This release enhances compatibility between litep2p and libp2p by using
the latest Yamux upstream version. Additionally, it includes various
improvements and fixes to boost the stability and performance of the
WebSocket stream and the multistream-select protocol.

### Changed

- yamux: Switch to upstream implementation while keeping the controller
API ([#320](paritytech/litep2p#320))
- req-resp: Replace SubstreamSet with FuturesStream
([#321](paritytech/litep2p#321))
- cargo: Bring up to date multiple dependencies
([#324](paritytech/litep2p#324))
- build(deps): bump hickory-proto from 0.24.1 to 0.24.3
([#323](paritytech/litep2p#323))
- build(deps): bump openssl from 0.10.66 to 0.10.70
([#322](paritytech/litep2p#322))

### Fixed

- websocket/stream: Fix unexpected EOF on `Poll::Pending` state
poisoning ([#327](paritytech/litep2p#327))
- websocket/stream: Avoid memory allocations on flushing
([#325](paritytech/litep2p#325))
- multistream-select: Enforce `io::error` instead of empty protocols
([#318](paritytech/litep2p#318))
- multistream: Do not wait for negotiation in poll_close
([#319](paritytech/litep2p#319))

cc @paritytech/networking

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
(cherry picked from commit 42e9de7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitry-markin dmitry-markin dmitry-markin approved these changes

Assignees

@lexnv lexnv

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lexnv @dmitry-markin