Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DataChannel max value for "id" before connecting? #2158

Closed
alvestrand opened this issue Apr 2, 2019 · 10 comments
Closed

DataChannel max value for "id" before connecting? #2158

alvestrand opened this issue Apr 2, 2019 · 10 comments
Assignees

Comments

@alvestrand
Copy link
Contributor

Current spec says:

  1. If [[DataChannelId]] is equal to 65535, which is greater than the maximum allowed ID of 65534 but still qualifies as an unsigned short, throw a TypeError.

  2. Let transport be the connection's [[SctpTransport]] slot.

If the [[DataChannelId]] slot is not null, transport is in the connected state and [[DataChannelId]] is greater or equal to the transport's [[MaxChannels]] slot, throw an OperationError.

If the implementation knows that it will propose a max channels smaller than 65535 in negotiation, this seems like a somewhat strange decision - even though we know that the number will be rejected, we're accepting it on channel creation.

Should this be changed?

@alvestrand
Copy link
Contributor Author

Tagging @lgrahl

@lgrahl
Copy link
Contributor

lgrahl commented Apr 2, 2019

rtcweb-data-channel-13, sec 6.2 says that implementations should allow for 65535 channels (with ids 0 to 65534):

 The number of streams negotiated during SCTP association setup SHOULD
 be 65535, which is the maximum number of streams that can be
 negotiated during the association setup.

IIRC from what @tuexen told me, the rationale was that renegotiation of the amount of streams would be unnecessarily complex (which I would agree with) and memory for those streams can be allocated on demand.

AFAIK current browser implementations only restrict the amount of data channels due to a limitation of usrsctp which allocates memory for each stream beforehand, see sctplab/usrsctp#121. And, at least on mobile browsers, having a couple of MiB (or so) per page that creates an RTCPeerConnection is not desirable.

That's the background


My thoughts: I think browsers should always support 65535 channels. Only other endpoints may use less than that (but they aren't targeted by the W3C API). I'm not strongly opposed to changing the spec slightly and say "[...] if the implementation allows less, throw X [...]" but I do see value in keeping it the way it is to nudge implementations to actually support 65535 channels and declare anything else proprietary.

@alvestrand
Copy link
Contributor Author

alvestrand commented Apr 2, 2019

The code in webrtc.org's code indicates that the memory footprint of increasing channels from the current 1024 to 65535 is worrisome:

// It's recommended to use the maximum of 65535 in:
// https://tools.ietf.org/html/draft-ietf-rtcweb-data-channel-13#section-6.2
// However, we use 1024 in order to save memory. usrsctp allocates 104 bytes
// for each pair of incoming/outgoing streams (on a 64-bit system), so 65535
// streams would waste ~6MB.

I haven't seen any bugs filed indicating that 1024 is seen as restrictive.

@lgrahl
Copy link
Contributor

lgrahl commented Apr 2, 2019

The ahead-of-use memory allocation overhead is an implementation problem. Just like the lack of ndata results in stream monopolisation and ignorance of stream prioritisation is an implementation problem. I'm not convinced the spec should work around these. Instead, it should aim at how it's supposed to be.

It's unfortunate that no one has tried solving sctplab/usrsctp#121 so far, yes.

@tuexen
Copy link

tuexen commented Apr 2, 2019

IIRC from what @tuexen told me, the rationale was that renegotiation of the amount of streams would be unnecessarily complex (which I would agree with) and memory for those streams can be allocated on demand.

The number of data channels is dynamic. The best way to deal with this is to use the capability of SCTP to increase the number of streams during the lifetime of an association. This was specified in earlier versions of the internet draft and implemented in Firefox, if I remember it correctly. However, the WebRTC WG decided that having the procedure to add dynamically streams to an existing association is too complex and just negotiating the maximum number is simpler. The memory overhead was considered not to be relevant. This was not my position, but the decision of the working group.
The problem with implementing a dynamic allocation is that you can't handle memory allocation failures, since you already negotiated that you support 65535 streams. So you can't change your mind. This was handled by the dynamic addition feature, which requested more streams and only if the resources are available, you negotiation would succeed. One could use an late allocation scheme where you only use sizeof(void *) for an unused incoming or outgoing stream. However, without impacting the performance too hard, one would always allocate the array of pointers for stream 0...N.
So using small stream IDs would be beneficial. Using a stream ID of 65535, would require the pointer array to be maximal. The question is what to do, if you can allocate the resources during the life time of an association. Any idea?

@alvestrand alvestrand self-assigned this Apr 4, 2019
@lgrahl
Copy link
Contributor

lgrahl commented Apr 5, 2019

First of all: I agree that WebRTC compatible endpoints (but not browsers) must be able to announce less than 64k streams. Definitely needed when talking to a small embedded device. But that's covered by the spec as it is today.

The problem with implementing a dynamic allocation is that you can't handle memory allocation failures, since you already negotiated that you support 65535 streams.

Realistically, if a browser encounters such a scenario where ~6 MiB (or something else along that magnitude) more would result in OOM, it's probably already doomed. 🙂

One could use an late allocation scheme where you only use sizeof(void *) for an unused incoming or outgoing stream. However, without impacting the performance too hard, one would always allocate the array of pointers for stream 0...N.

Hrm, I can't quite follow here. Enlarging an array should be easy enough? Sure, it needs an additional size variable, but other than that?

So using small stream IDs would be beneficial. Using a stream ID of 65535, would require the pointer array to be maximal.

A hash table based lookup may also be something feasible to keep the memory impact low with high stream ids.

Generally I don't consider it an issue. Applications who use stream ids in the area of 64k will be extremely rare. Even if it's a pointer array implementation it's only ~512 KiB on 64-bit machines to enlarge the stream array to 65535 void pointers.

(Side note: Yeah, renegotiation of the number of streams is still implemented in Firefox... although AFAIK it's still a bit buggy here and there, so the complexity argument isn't that far off.)

@alvestrand
Copy link
Contributor Author

My current worry about the negotiation thing is that when you create a datachannel that requires negotiation, the id will have to remain -1 and the state remian "opening" until the negotiation is completed - after which it will transition to "open" or "closed" depending on the outcome.

It is reasonable to think that since this is a rare case, people will forget to program for it.

@lgrahl
Copy link
Contributor

lgrahl commented Apr 6, 2019

The maximum amount of streams is something that will always be negotiated during the SCTP handshake. And since we can't assume the other endpoint is a browser, what you worry about is something that can and will happen.

For example, the browser supports 64k streams but the remote is something tiny and only supports 8 inbound and 8 outbound streams.

  1. Calling createDataChannel('foo') five times will succeed before the handshake completed. id will be null for all of them.
  2. Once the DTLS role has been established (after offer/answer), id will be populated by following the odd/even rule. For example: 0, 2, 4, 6 and 8. (This step isn't crystal clear in the spec.)
  3. Once the SCTP association has been established, go through each data channel and fire close and error on those whose id >= sctp.maxChannels. So, channel with id 8 will be closed. It will never open.

As you can see, even if the browser supports 64k streams, this is something inevitable. If a browser supports less than that (albeit not spec-compliant in my eyes), it would be the exact same procedure as that would be ultimatively reflected by sctp.maxChannels.

Edit: Once we have established an SCTP association, createDataChannel will do the id >= sctp.maxChannels check in step 20.

@alvestrand
Copy link
Contributor Author

I'm closing this as "no change needed". There doesn't seem to be a compelling reason to change the spec; the code will have to deal with negotiating down the number of channels anyway, and we can hope that the present case (where Chrome supports 1024 channels) is not permanent.

@stalkerg
Copy link

Ok made tests and can share result:
Max id for Chrome is 1024, for Firefox is 256.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants