search

vacp2p / nim-libp2p

libp2p implementation in Nim
https://vacp2p.github.io/nim-libp2p/docs/
MIT License
242 stars 52 forks source link

avoid `KeyError` in edge case of yamux handler #1044

Closed etan-status closed 4 months ago

etan-status commented 4 months ago

There are two edge cases that currently trigger KeyError:

  1. await newStream.open() succeeds, but Chronos injects an implicit suspend point when an {.async.} function that suspended returns. During this implicit suspend point, another task could get scheduled and close the stream, which removes it from m.channels again. let channel = m.channels[header.streamId then gives KeyError. More explanation of implicit suspend point on return after await: https://github.com/status-im/nim-chronos/pull/273

  2. During the Flush the data block, await m.connection.readExactly is called which may suspend. If it suspends, same as above, another task may get scheduled that closes the stream, also resulting in the KeyError below.

The YamuxError is subsequently properly handled below.

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 82.76%. Comparing base (6c87348) to head (a7fe0b4).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/status-im/nim-libp2p/pull/1044/graphs/tree.svg?width=650&height=150&src=pr&token=UR5JRQ249W&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=status-im)](https://app.codecov.io/gh/status-im/nim-libp2p/pull/1044?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=status-im) ```diff @@ Coverage Diff @@ ## unstable #1044 +/- ## ============================================ - Coverage 82.77% 82.76% -0.01% ============================================ Files 91 91 Lines 15604 15609 +5 ============================================ + Hits 12916 12919 +3 - Misses 2688 2690 +2 ``` | [Files](https://app.codecov.io/gh/status-im/nim-libp2p/pull/1044?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=status-im) | Coverage Δ | | |---|---|---| | [libp2p/muxers/yamux/yamux.nim](https://app.codecov.io/gh/status-im/nim-libp2p/pull/1044?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=status-im#diff-bGlicDJwL211eGVycy95YW11eC95YW11eC5uaW0=) | `87.96% <60.00%> (-0.38%)` | :arrow_down: |