fix(yamux): set EoF when remote peer half closes the stream in yamux #1086
Conversation
diegomrsantos
force-pushed
the
ping-test-error
branch
from
caf4625
to
f8a27a2
Compare
diegomrsantos
force-pushed
the
ping-test-error
branch
from
f8a27a2
to
760cb78
Compare
This reverts commit f1c39bd.
diegomrsantos
changed the title
diegomrsantos
force-pushed
the
ping-test-error
branch
from
4f8c486
to
8cdc5a1
Compare
libp2p/muxers/yamux/yamux.nim
Outdated
| @@ -442,7 +441,7 @@ proc cleanupChannel(m: Yamux, channel: YamuxChannel) {.async: (raises: []).} = | |||
| when defined(libp2p_yamux_metrics): | |||
| libp2p_yamux_channels.set( | |||
| m.lenBySrc(channel.isSrc).int64, [$channel.isSrc, $channel.peerId]) | |||
| if channel.isReset and channel.recvWindow > 0: | |||
| if (channel.isReset or not channel.closedRemotely.completed()) and channel.recvWindow > 0: | |||
There was a problem hiding this comment.
I'm not sure about this new condition.
There was a problem hiding this comment.
channel.closedRemotely is always completed after channel.join().
In theory, the only possible case is if we try to cancel, but cleanupChannel is async spawned, so we cannot cancel it.
There was a problem hiding this comment.
channel.closedRemotely is always completed after channel.join().
Is it? remoteClosed is called only in reset and when a fin is received. It'd be probably better to rename this proc to a verb.
There was a problem hiding this comment.
channel.join() is completed after LPstream.closeImpl() is called.
LPStream.closeImpl() is called by actuallyClosed here, and it's called after checking if closedRemotely is completed.
There was a problem hiding this comment.
True, reverting.
diegomrsantos
force-pushed
the
ping-test-error
branch
from
8b5874c
to
3d6deed
Compare
diegomrsantos
force-pushed
the
ping-test-error
branch
from
3d6deed
to
3b038f9
Compare
diegomrsantos
changed the title
| @@ -11,6 +11,6 @@ COPY . nim-libp2p/ | |||
|
|
|||
| RUN \ | |||
| cd nim-libp2p && \ | |||
| nim c --skipProjCfg --skipParentCfg --NimblePath:./nimbledeps/pkgs -p:nim-libp2p -d:chronicles_log_level=WARN --threads:off ./tests/transport-interop/main.nim | |||
| nim c --skipProjCfg --skipParentCfg --NimblePath:./nimbledeps/pkgs -p:nim-libp2p -d:chronicles_log_level=WARN -d:chronicles_default_output_device=stderr --threads:off ./tests/transport-interop/main.nim | |||
There was a problem hiding this comment.
This is how it should be according to the test spec in test-plans. This is a bit unrelated to the current scope of the PR, but the PR itself was born from debugging this test and this change was necessary, otherwise, the test fails when increasing the log level. Ideally, it should be done in a different PR, but it is fine in this case.
returnedEoFfield