search

libp2p / cpp-libp2p

C++17 implementation of libp2p
Apache License 2.0
347 stars 98 forks source link

YamuxStream is tagged to Unreadable too early, since there may be data that hasn't been processed #167

Open Saigut opened 2 years ago

Saigut commented 2 years ago

From testing of my program, after YamuxStream tagging stream being unreadable in onFINReceived at this line, there may still be remaining data to be processed in onLengthRead. So I think tagging stream being unreadable in onFINReceived is too early.

I have no better idea for now, so I just comment this line to workaround this issue.

creativeid00 commented 2 years ago

I would say that it is more like the stream acceptance callback is called too late. I got the same issue during bitswap message receiving from a remote go-ipfs peer. The go-ipfs peers sends 3 frames:

  1. frame.type WINDOW_UPDATE (1 '\x1') frame.length 0 frame.flags 1
  2. frame.type DATA (0 '\0') frame.length 248 frame.flags 0
  3. frame.type WINDOW_UPDATE (1 '\x1') frame.length 0 frame.flags 4 The last frame contains FIN flag that leads to onFINReceived call. Protocol handler is not called neither after the 1st frame nor after the second one. It is called when the last frame received, some response sent to the remote ipfs peer and Multiselect instance is closed. By that time the yamux stream has already been tagged as Unreadable.
creativeid00 commented 2 years ago

Detailed investigation has shown that YamuxStream::doRead returns corresponding error https://github.com/libp2p/cpp-libp2p/blob/91d58f28bb8cbce06fd717082fa2b4274a33ae07/src/muxer/yamux/yamux_stream.cpp#L447 only when no pending data is available in the internal_readbuffer. https://github.com/libp2p/cpp-libp2p/blob/91d58f28bb8cbce06fd717082fa2b4274a33ae07/src/muxer/yamux/yamux_stream.cpp#L425 I'm not sure what is a semantics of YamuxStream::isClosedForRead method but it looks like the size of pending data should be checked within the method also.