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

Throw error if data channel's buffer is filled, rather than closing. #1209

Merged
merged 4 commits into from
Jun 15, 2017

Conversation

taylor-b
Copy link
Contributor

Fixes #1148.

Option 2 for fixing the issue. The application doesn't need to know the
buffer's size ahead of time, because filling the buffer is no longer an
irrecoverable error. If it doesn't care about latency, an application
could send until an exception is thrown, then wait for the
"bufferedamountlow" event until it can send again, similar to a
non-blocking socket.

Note that this is deliberately different than how WebSockets work,
because the circumstances are slightly different: with WebSockets, it
was expected that applications had code to handle them being closed
unexpectedly. But this isn't true of WebRTC DataChannels, because before
an SCTP-level timeout can occur, the application would have done an ICE
restart and repaired the problem.

Fixes w3c#1148.

Option 2 for fixing the issue. The application doesn't need to know the
buffer's size ahead of time, because filling the buffer is no longer an
irrecoverable error. If it doesn't care about latency, an application
could send until an exception is thrown, then wait for the
"bufferedamountlow" event until it can send again, similar to a
non-blocking socket.

Note that this is deliberately different than how WebSockets work,
because the circumstances are slightly different: with WebSockets, it
was expected that applications had code to handle them being closed
unexpectedly. But this isn't true of WebRTC DataChannels, because before
an SCTP-level timeout can occur, the application would have done an ICE
restart and repaired the problem.
webrtc.html Outdated
data transport</a> <a>with an error</a>.</p>
<a>underlying data transport</a>. If the data cannot be sent, buffer
it, and if this is not possible because the buffer is full,
<a>throw</a> an <code>OperationError</code>.</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if these are the right errors; suggestions welcome.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The WebIDL spec [1] says that InvalidAccessError is deprecated.

[1] https://heycam.github.io/webidl/#invalidaccesserror

@stefhak
Copy link
Contributor

stefhak commented May 18, 2017

Interim topic to decide between options? And are they exclusive?

@taylor-b
Copy link
Contributor Author

They're not necessarily mutually exclusive, no.

@aboba
Copy link
Contributor

aboba commented Jun 8, 2017

Related to Issue #1287

@soareschen
Copy link
Contributor

soareschen commented Jun 12, 2017

I think there are few clarifications needed:

  • When there are already buffers queued, it should attempt to queue the buffer without attempting to send to underlying data transport.
  • If there is error sending to underlying transport, should there be error thrown? Or should the interaction with underlying transport be asynchronous and queued as a task?
  • If readyState is closing or closed, why is that error ignored while others are thrown?

@taylor-b
Copy link
Contributor Author

  • When there are already buffers queued, it should attempt to queue the buffer without attempting to send to underlying data transport.

Good catch... Reworded to make this clear.

  • If there is error sending to underlying transport, should there be error thrown? Or should the interaction with underlying transport be asynchronous and queued as a task?

It should always be asynchronous. That way the application only has one thing to listen to for SCTP protocol level errors: onerror.

  • If readyState is closing or closed, why is that error ignored while others are thrown?

I don't see how send could work if readyState is "closing" or "closed". I changed the readyState != "opening" check to readyState == "open".

@soareschen
Copy link
Contributor

soareschen commented Jun 15, 2017

I don't see how send could work if readyState is "closing" or "closed". I changed the readyState != "opening" check to readyState == "open".

👏

LGTM. This should also fix #1376 since it is now clear than actual data transmission is done in parallel.

webrtc.html Outdated
<li>
<p>Queue <var>data</var> for transmission on <var>channel</var>'s
<a>underlying data transport</a>. If queuing <var>data</var> is not
possible because no more buffer space is available, <a>throw</a> an
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/no/not enough/ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@stefhak
Copy link
Contributor

stefhak commented Jun 15, 2017

Made a comment, but I think we can merge now (regardless to whether or not we change based on that comment).

webrtc.html Outdated
data transport</a> <a>with an error</a>.</p>
<a>underlying data transport</a>. If the data cannot be sent, buffer
it, and if this is not possible because the buffer is full,
<a>throw</a> an <code>OperationError</code>.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The WebIDL spec [1] says that InvalidAccessError is deprecated.

[1] https://heycam.github.io/webidl/#invalidaccesserror

webrtc.html Outdated
<p>If the size of <var>data</var> exceeds the value of <code><a
data-link-for="RTCSctpTransport">maxMessageSize</a></code> on
<var>channel</var>'s associated <code>RTCSctpTransport</code>,
<a>throw</a> an <code>InvalidAccessError</code>.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should go with an operation error here as well since InvalidAccessError is deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. What's the new "invalid parameter" error, then?

@aboba aboba merged commit 51159df into w3c:master Jun 15, 2017
lgrahl added a commit to lgrahl/ortc that referenced this pull request Jun 30, 2017
Remove unneeded conditions. (w3c/webrtc-pc#1404)
Throw an error if data channel's buffer is filled rather than implicit closing. (w3c/webrtc-pc#1209)
Throw an error if len(message) > max-message-size of the remote peer (w3c/webrtc-pc#1209)
@lgrahl
Copy link
Contributor

lgrahl commented Oct 26, 2017

We have added this in Firefox 57. I'm currently writing a shim in adapter-js. Any other implementations that support this I should to be aware of (preferrably with version number)?

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

Successfully merging this pull request may close these issues.

How do applications know a DataChannel's buffer capacity, so they can avoid filling it?
6 participants