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

[WIP] Fix eof on read #18

Merged
merged 1 commit into from
May 7, 2019
Merged

[WIP] Fix eof on read #18

merged 1 commit into from
May 7, 2019

Conversation

enobufs
Copy link
Member

@enobufs enobufs commented May 3, 2019

Description

Currently, on receipt of EOF pion/datachannel closes the outgoing channel.

It is the requirement from libp2p that even EOF is received on a datachannel, the datachannel be available for write until the app calls Close on the datachannel. (in the context of detached mode)

The sctp branch this PR uses solves EOF related problems. This fix in datachannel module will complete the whole issue around EOF discussed in pion/webrtc#652.

Reference issue

Fixes pion/webrtc#652

TODO

Before merging this, we will need to tag sctp this PR depends on.

Copy link

@albrow albrow left a comment

Choose a reason for hiding this comment

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

This LGTM but I would wait for @backkem too since he has more context.

datachannel.go Outdated
@@ -175,15 +175,8 @@ func (c *DataChannel) Read(p []byte) (int, error) {
func (c *DataChannel) ReadDataChannel(p []byte) (int, bool, error) {
for {
n, ppi, err := c.stream.ReadSCTP(p)
if err == io.EOF {
Copy link
Member

Choose a reason for hiding this comment

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

Are we required to lose this? It's defined in the data channel spec:
https://tools.ietf.org/html/draft-ietf-rtcweb-data-channel-13#section-6.7

Closing of a data channel MUST be signaled by resetting the
corresponding outgoing streams [RFC6525]. This means that if one
side decides to close the data channel, it resets the corresponding
outgoing stream. When the peer sees that an incoming stream was
reset, it also resets its corresponding outgoing stream. Once this
is completed, the data channel is closed.

Copy link
Member Author

@enobufs enobufs May 7, 2019

Choose a reason for hiding this comment

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

Hmmm. I may have read it wrong.

When the peer sees that an incoming stream was reset, it also resets its corresponding outgoing stream. 

In this sentence, "the peer" is referred to SCTP/DataChannel entity or application?

In TCP, when a node receives FIN, it goes into CLOSE_WAIT state in which application can still send data, so how I read made sense but now I am not sure: https://upload.wikimedia.org/wikipedia/commons/f/f6/Tcp_state_diagram_fixed_new.svg

If we did not make this change, TestEOF (in pion/webrtc) would fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, with non-detach case, if a node receives EOF on a datachannel, OnClose callback is made. I guess it is a bit weird to have the channel actually half-open after OnClose is made... How about enabling this half-open (CLOSE_WAIT) state only with detached datachannel?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

@enobufs enobufs May 7, 2019

Choose a reason for hiding this comment

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

rfc6458 does not seem to assume rfc6525 (reconfig) because it is true that SCTP (rfc4960 alone) only has Shutdown or Abort of association. rfc6525 is pretty close to allow TCP socket's SHUT_WR operation, but again the above sentence is still a question...

Copy link
Member

Choose a reason for hiding this comment

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

Regarding half-open (CLOSE_WAIT) state: a data channel is defined as bidirectional: https://tools.ietf.org/html/draft-ietf-rtcweb-data-channel-13#section-6.4

Data channels are defined such that their accompanying application-
level API can closely mirror the API for WebSockets, which implies
bidirectional streams of data and a textual field called 'label' used
to identify the meaning of the data channel.

Same on the protocol level: https://tools.ietf.org/html/draft-ietf-rtcweb-data-protocol-09#section-4

The Data Channel Establishment Protocol is a simple, low-overhead way
to establish bidirectional Data Channels over an SCTP association
with a consistent set of properties.

Therefore, It seems unlikely that this use-case would come up.

Copy link
Member

Choose a reason for hiding this comment

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

Re-opened. @enobufs let us know if you agree.

@backkem backkem merged commit 4101595 into master May 7, 2019
@backkem backkem deleted the fix-eof-on-read branch May 7, 2019 19:26
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

Successfully merging this pull request may close these issues.

EOF returns earlier than the last message on receipt of Reset
3 participants