mss.handle never resolves when optimisticSelect
#2321
Comments
|
Removing this line makes everything work again. So it feels that there is some funky concurrency issue perhaps where |
marcus-pousette
changed the title
mss.handle never resolves for incomingStreammss.handle never resolves when optimisticSelect
|
#2318 resolves a problem where for very short lived streams, where we send a small amount of data and then immediately close the stream, we'd end up closing the stream before negotiation had finished. Is this similar to what you are doing? Or something else? If it's something else can you link me to an example? |
I did try to patch my lib with those changes but where not successful in eliminating the problem. Only by remove the I am doing a similiar thing as to how gossipsub handles and setups new streams, so I am curious how no other have got this problem yet (but it seems that gossipsub project only tests their protocol on a mocked client). usually I receive incoming streams. But no I no longer receive them because Things that might differ from my setup to others: I am using the circuit relay v2 and identify protocol. And both sides tries to open a stream to the other side at the same time once they have successfully established a connection (on topology changes |
|
I also tried added a 5s delay before opening a stream on a new connection to see whether the issues where related to other protocols needing to go through the |
|
Here I enter with /noise But I only seem to send protocol for Interestingly at the exact moment my test fail (i.e. it starts to close the outbound stream of my protocol handler (?)) then it starts to send /test/0.0.0 because I reach here but fails with the error It is like that "/test/0.0.0" gets stuck at somehowI am testing now with the newest release (1.0.11) |
|
I wonder if the race condition here is that both sides expect the other side to write some data, so no-one starts to negotiate before its too late? |
|
I think I have figured out the issue now: Previously a stream (inbound and outbound) could be setup without any data to be written, this allowed one to dial and wait for this to happen without engaging in any activity. This is something I did rely on at various places, since I assumed previously a half-opened stream would be considered, not ready, unwanted or a sign that something has gone wrong. The case now is that i have to write data in order to setup a stream fully (read and write). I.e my apps has to trust half-open streams to be 'ok' even though perhaps something can go wrong down the line I need to understand though if I treat a "stream is 'ready'" requirement only to be that the outbound direction only exist, will not create any problems further down the line. Like that if the inbound stream is never successfully setup, I need a way of treating the whole stream as 'not ready'. This kind of creates a scenario where I need to write some code to correctly handle the case where a half-open stream never succesfully is able to convert fully open stream, and terminate the stream altogether. |
|
This sounds like it's getting closer. From the recent performance work it became apparent that round trips are the thing that make opening connections slow. When there's only a single protocol being negotiated, in the case of open stream & write data, we optimistically write the protocol and the first chunk of data into the transport together, saving a round trip of sending the protocol, reading the response, then sending the data - this is what lowers connection establisment time to around 200ms. Of course it needs the first data chunk to do this, so negotiation doesn't complete until the stream is interacted with. In the case of open stream & read data it falls back to send the protocol, read the response, read some data, but only when the caller tries to read from the stream - so again negotiation doesn't complete until the stream is interacted with. When there are multiple protocols being negotiated it has the older behaviour of immediately writing/reading protocol names/responses until one is agreed upon, since we can't send protocol+data, then send another protocol because the remote will interpret the data as the next protocol to be negotiated. The intention here is that you open a stream, try to write or read data and deal with any failures as they occur. If the case is that you need to open streams but do nothing with them, we might need to add an option to allow ensuring they are negotiated fully immediately to support this. |
Adds an option to `libp2p.dialProtocol` and `connection.newStream` to disable optimisitic protocol selection and default to waiting for negotation to finish before sending data. This increases the time-to-first-byte of a stream negotiated with a single protocol but restores the previous behaviour. Fixes #2321
|
Make sense!
I wonder whether there is a scenario where not doing optimistic protocol selection is faster. Imagine a case where you dial a bunch of peers then, eventually you want to send data to them. In this case, there might be idle time before data is sent, and that time could be used to negotiate the protocols.. If you in this case do protocol selection at the time the first data is sent, the delivery of the first package will be slower than if the negotiation was done prior I imagine this (pseudo code) to solve both scenarios if (protocols length === 1 )
{
optimisticProtocolSelection()
setTimeout(()=>{
normalSelection()
},100)
} |
|
Another thing, this line Seem to always make my code throw/log the error "Cannot push value onto an ended pushable" if have a created a stream but never used it and is terminating it. I expect that there would be no errors thrown if I terminate a stream without sending any data through it. What is the purpose of negotiating the protocols that is "ending"? |
|
It's possible that we don't need that line now since #2318 has made closing streams in mid-negotiation a bit more predictable. |
Adds an option to `libp2p.dialProtocol` and `connection.newStream` to enable optimistic protocol selection but defaults to waiting for negotiation to finish before sending data. This increases the time-to-first-byte of a stream negotiated with a single protocol but restores the previous behaviour. Fixes #2321 --------- Co-authored-by: Chad Nehemiah <chad.nehemiah94@gmail.com>
libp2p: 1.0.10
Darwin MacBook-Pro-3 23.0.0
multistream-select (?)
Severity:
Critical/High
Description:
I am in the process of upgrading to v1 from 0.46. After upgrading I am no longer able to receive incoming streams in my protocol handler. Something that did work prior to upgrading the libp2p version.
Specifically, the registrar handle never emits any new incoming streams.
Going through this more, it seems that I get incoming streams, but the code never resolves at
mss.handlejs-libp2p/packages/libp2p/src/upgrader.ts
Line 373 in ad6f70b
I get the same results when using yamux and mplex.
These are the logs I get when both nodes attempt to open a stream to the other node at the same time
I wonder whether #2318 is intended to fix this issue. Or some other work related to 'optimistic' stream selection is related to this.
The text was updated successfully, but these errors were encountered: