[Yamux] Allow max ACK backlog of 256 streams #340

StefanBratanov · 2023-10-24T14:08:07Z

https://github.com/libp2p/specs/blob/master/yamux/README.md#ack-backlog--backpressure

Implements SHOULD at most allow an ACK backlog of 256 streams.

Added new parameter ackBacklogLimit when initializing Yamux
Introduced notion of acknowledged when initializing yamux stream
throw and don't create new stream if more than 256 streams are not acknowledged

mxinden · 2023-10-24T14:48:05Z

//CC @thomaseizinger who pushed a lot for this to land in rust-libp2p and specs.

Nashatyrev · 2023-10-25T06:08:47Z

Just to make sure I'm getting the Yamux spec correctly:

There could be two kinds of unacknowledged streams:

outbound: local peer opens a stream, sends SYN, but hasn't received ACK yet
inbound: in our current implementation we are sending back ACK immediately upon receiving a SYN from a remote party. In that case there is no way to have unacknowledged inbound streams. However the spec states the following:
MAY delay acknowledging new streams until the application has received or is about to send the first DATA frame.
If this statement is implemented at some point then a remotely opened stream may also be unacknowledged. I.e. until the first inbound data frame is received or a local app starts sending a data.

So the questions:

should we account both types as a sum and treat them equally?
In the case of exceeding unacknowledged streams threshold what should we do when ... opening a new stream locally? Should we throw MuxerWriteException? (i think yes) Should we also abort the connection? (probably no)
In the case of exceeding unacknowledged streams threshold what should we do when ... receiving SYN from remote? Abort the stream (by sending back RST)? Abort the connection?

Nashatyrev

Some questions remain

Nashatyrev · 2023-10-25T06:10:12Z

libp2p/src/main/kotlin/io/libp2p/mux/yamux/YamuxHandler.kt

 import java.util.concurrent.atomic.AtomicInteger
 import kotlin.math.max
 import kotlin.properties.Delegates

+const val ACK_BACKLOG_ALLOWANCE = 256


I would parametrize it for the YamuxHandler making this constant default value

Nashatyrev · 2023-10-25T06:12:40Z

libp2p/src/main/kotlin/io/libp2p/mux/yamux/YamuxHandler.kt

-        createYamuxStreamHandler(child.id).onLocalOpen()
+        if (totalUnacknowledgedStreams() < ACK_BACKLOG_ALLOWANCE) {
+            createYamuxStreamHandler(child.id).onLocalOpen()
+        }


Should we throw a specific WriteMuxerException here?

libp2p/src/main/kotlin/io/libp2p/mux/yamux/YamuxHandler.kt

mxinden · 2023-10-25T09:34:37Z

should we account both types as a sum and treat them equally?

They should be accounted separately. A should not ACK inbound streams from B, in case A's application layer has not yet read or written to a stream. B should not open a new stream to A in case 256 outbound streams to A have not yet been ACKed. Same vice versa for streams from A to B.

mxinden · 2023-10-25T09:35:26Z

In the case of exceeding unacknowledged streams threshold what should we do when ... opening a new stream locally? Should we throw MuxerWriteException? (i think yes) Should we also abort the connection? (probably no)

Do you have any way to backpressure the application? In Rust and I am assuming in Go as well, we suspend the green thread.

mxinden · 2023-10-25T09:38:41Z

In the case of exceeding unacknowledged streams threshold what should we do when ... receiving SYN from remote? Abort the stream (by sending back RST)? Abort the connection?

In rust-yamux today it is best effort only, enforced at the outbound end for now.

https://github.com/libp2p/rust-yamux/blob/93b70626f7f3d83f97951d9cd5c2951500088f0d/yamux/src/connection.rs#L435-L439

In case you want to enforce it at the receiving side, I would consider resetting the stream exceeding the limit.

Nashatyrev · 2023-10-25T12:06:05Z

Do you have any way to backpressure the application?

Not with the present API

In Rust and I am assuming in Go as well, we suspend the green thread.

Yeah, that's a good solution.

In case you want to enforce it at the receiving side, I would consider resetting the stream exceeding the limit.

If both peers has the same ACK_BACKLOG_ALLOWANCE then this situation should be treated as the protocol violation. Am I getting it right?
In that case even connection abort would be appropriate

mxinden · 2023-10-25T14:21:33Z

In case you want to enforce it at the receiving side, I would consider resetting the stream exceeding the limit.

If both peers has the same ACK_BACKLOG_ALLOWANCE then this situation should be treated as the protocol violation. Am I getting it right?

Correct. Though I am not aware of a single libp2p based network out there today that supports the yamux ack backlog feature across all nodes. Thus treating it as best effort everywhere.

Nashatyrev · 2023-10-25T14:43:23Z

libp2p/src/main/kotlin/io/libp2p/mux/yamux/YamuxHandler.kt

@@ -240,8 +245,11 @@ open class YamuxHandler(
        streamHandlers.remove(child.id)?.dispose()
    }

-    private fun calculateTotalBufferedWrites(): Int {
-        return streamHandlers.values.sumOf { it.sendBuffer.readableBytes() }
+    override fun checkCanOpenNewStream() {


Why not just throw from onLocalOpen() ?

So essentially in newStream method in AbstractMuxHandler, if onLocalOpen fails then outboundInitializer is never called and createStream in MuxHandler controller future is never complete. And I am thinking also in order to avoid calling createChild with he new id.

if onLocalOpen fails then outboundInitializer is never called and createStream in MuxHandler controller future is never complete.

Right that probably should be fixed. register() doesn't rethrow exceptions but rather wraps them into ChannelFuture, so the best way is to call sync() to rethrow. register() call shouldn't block so the sync() should be synchronous:

Index: libp2p/src/main/kotlin/io/libp2p/etc/util/netty/mux/AbstractMuxHandler.kt IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/libp2p/src/main/kotlin/io/libp2p/etc/util/netty/mux/AbstractMuxHandler.kt b/libp2p/src/main/kotlin/io/libp2p/etc/util/netty/mux/AbstractMuxHandler.kt --- a/libp2p/src/main/kotlin/io/libp2p/etc/util/netty/mux/AbstractMuxHandler.kt (revision 1927bcae6804fc5b24662a40221e217b95298944) +++ b/libp2p/src/main/kotlin/io/libp2p/etc/util/netty/mux/AbstractMuxHandler.kt (date 1698247944849) @@ -138,7 +138,7 @@ ): MuxChannel<TData> { val child = MuxChannel(this, id, initializer, initiator) streamMap[id] = child - ctx!!.channel().eventLoop().register(child) + ctx!!.channel().eventLoop().register(child).sync() return child }

And I am thinking also in order to avoid calling createChild with he new id.

Doesn't look like a big issue: the stream seems to immediately disposed in that case and removed from the stream map

With the fix above throwing from onLocalOpen() worked for me

Nice. I like it. Done.

Nashatyrev · 2023-10-26T08:43:56Z

It also looks like we need to treat initiator/responder acknowledgments separately as per comment from @mxinden

@StefanBratanov are you planning to address this here yet or later?
Else the PR looks good to me.

StefanBratanov · 2023-10-26T09:07:11Z

It also looks like we need to treat initiator/responder acknowledgments separately as per comment from @mxinden

@StefanBratanov are you planning to address this here yet or later? Else the PR looks good to me.

Working on it now. Think it makes sense to be in this PR as well.

StefanBratanov · 2023-10-26T10:13:57Z

It also looks like we need to treat initiator/responder acknowledgments separately as per comment from @mxinden

@StefanBratanov are you planning to address this here yet or later? Else the PR looks good to me.

Working on it now. Think it makes sense to be in this PR as well.

Done.

Nashatyrev

LGTM, just some nits 👍

libp2p/src/main/kotlin/io/libp2p/mux/yamux/YamuxHandler.kt

[Yamux] Allow max ACK backlog of 256 streams

9581f89

Improvements

51ac475

Nashatyrev reviewed Oct 25, 2023

View reviewed changes

Do not create new stream if ack backlog has been reached

1927bca

StefanBratanov marked this pull request as ready for review October 25, 2023 14:30

Nashatyrev reviewed Oct 25, 2023

View reviewed changes

ianopolous mentioned this pull request Oct 25, 2023

Yamux SHOULD NOT implicitly acknowledge streams but wait for the application to send the first DATA frame. #287

Open

StefanBratanov added 4 commits October 25, 2023 16:48

changes as per feedback

85789c0

cleaner way

93dbf04

rename for consistency

b33a730

remove unused import

b330512

Treat responder acknowledgments separately

d16d1f0

Nashatyrev approved these changes Oct 26, 2023

View reviewed changes

libp2p/src/main/kotlin/io/libp2p/mux/yamux/YamuxHandler.kt Outdated Show resolved Hide resolved

libp2p/src/main/kotlin/io/libp2p/mux/yamux/YamuxHandler.kt Outdated Show resolved Hide resolved

StefanBratanov added 2 commits October 26, 2023 13:19

nit

706cefa

into method

8cd79b0

StefanBratanov enabled auto-merge (squash) October 26, 2023 12:23

StefanBratanov merged commit e363c49 into libp2p:develop Oct 26, 2023

StefanBratanov deleted the max_ack_backlog branch November 19, 2023 10:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Yamux] Allow max ACK backlog of 256 streams #340

[Yamux] Allow max ACK backlog of 256 streams #340

StefanBratanov commented Oct 24, 2023 •

edited

Loading

mxinden commented Oct 24, 2023

Nashatyrev commented Oct 25, 2023

Nashatyrev left a comment

Nashatyrev Oct 25, 2023

Nashatyrev Oct 25, 2023

mxinden commented Oct 25, 2023

mxinden commented Oct 25, 2023

mxinden commented Oct 25, 2023

Nashatyrev commented Oct 25, 2023

mxinden commented Oct 25, 2023

Nashatyrev Oct 25, 2023

StefanBratanov Oct 25, 2023

Nashatyrev Oct 25, 2023

Nashatyrev Oct 25, 2023

StefanBratanov Oct 25, 2023 •

edited

Loading

Nashatyrev commented Oct 26, 2023

StefanBratanov commented Oct 26, 2023

StefanBratanov commented Oct 26, 2023

Nashatyrev left a comment

[Yamux] Allow max ACK backlog of 256 streams #340

[Yamux] Allow max ACK backlog of 256 streams #340

Conversation

StefanBratanov commented Oct 24, 2023 • edited Loading

mxinden commented Oct 24, 2023

Nashatyrev commented Oct 25, 2023

Nashatyrev left a comment

Choose a reason for hiding this comment

Nashatyrev Oct 25, 2023

Choose a reason for hiding this comment

Nashatyrev Oct 25, 2023

Choose a reason for hiding this comment

mxinden commented Oct 25, 2023

mxinden commented Oct 25, 2023

mxinden commented Oct 25, 2023

Nashatyrev commented Oct 25, 2023

mxinden commented Oct 25, 2023

Nashatyrev Oct 25, 2023

Choose a reason for hiding this comment

StefanBratanov Oct 25, 2023

Choose a reason for hiding this comment

Nashatyrev Oct 25, 2023

Choose a reason for hiding this comment

Nashatyrev Oct 25, 2023

Choose a reason for hiding this comment

StefanBratanov Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Nashatyrev commented Oct 26, 2023

StefanBratanov commented Oct 26, 2023

StefanBratanov commented Oct 26, 2023

Nashatyrev left a comment

Choose a reason for hiding this comment

StefanBratanov commented Oct 24, 2023 •

edited

Loading

StefanBratanov Oct 25, 2023 •

edited

Loading