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

Clarification on a stream receiving a FIN bit #36

Merged
merged 8 commits into from
Aug 17, 2018
Merged

Conversation

aboba
Copy link
Collaborator

@aboba aboba commented Aug 8, 2018

Fix for Issue #30

@aboba aboba changed the title Clarification on FIN bit receiving on a stream Clarification on a stream receiving a FIN bit Aug 8, 2018
@aboba aboba requested a review from pthatcherg August 8, 2018 19:18
@aboba
Copy link
Collaborator Author

aboba commented Aug 8, 2018

@shampson Can you take a look?

@aboba aboba requested a review from shampson August 9, 2018 18:55
index.html Outdated
<li> The QUIC stream has received STREAM frame with the FIN flag set.</li>
for transmission, the write buffer to be cleared and <var>stream</var>'s
<a>[[\Writeable]]</a> slot to be set to <code>false</code>.</li>
<li><var>stream</var> has <a>finished reading</a>.</li>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this set [[Readable]] to false?

Copy link
Collaborator Author

@aboba aboba Aug 15, 2018

Choose a reason for hiding this comment

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

Calling finish() causes a transition to closing with the [[Writeable]] slot false, but the [[Readable]] slot remains true until a FIN or RST_STREAM is received. This is shown in Figure 1.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right. This is saying we transition to closing when we've "finished reading," meaning if the stream has "read all available data up to the STREAM frame with the FIN bit set."

I'm asking if at that point the [[Readable]] slot should transition to false (because there's nothing to read anymore). If we also need to wait for finish() to be called, then it can stay as is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it should. Added text to make that clear.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good 👍

index.html Outdated
@@ -953,7 +954,7 @@ <h2>Methods</h2>
slots to <code>false</code>.</li>
<li>Set <var>stream</var>'s <a>[[\QuicStreamState]]</a> slot to
<code>closing</code>.</li>
<li> If the <var>stream</var> has received a STREAM frame with the FIN bit set,
<li>If <var>stream</var> has <a>finished reading</a>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this make sense? t seems to me that if you've received a FIN bit at this point you should transition to close, and it shouldn't depend on finishing reading. You can't read after this point anyways (reset() changes [[Readable]] to false).

This also contradicts 2. in the closed definition.

Copy link
Collaborator Author

@aboba aboba Aug 15, 2018

Choose a reason for hiding this comment

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

Reverted the change in line 956/957. Does it make sense now?

<p>The QUIC stream has been closed or could not be established.
This can happen in multiple ways:</p>
<p>The <code><a>RTCQuicStream</a></code> <var>stream</var> has been
closed or could not be established. This can happen in multiple ways:</p>
<ol>
<li>A RST_STREAM frame has been received.</li>
<li>A RST_STREAM frame has been sent and a STREAM frame
Copy link
Collaborator

Choose a reason for hiding this comment

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

This bullet contradicts your change to reset().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Change to reset() reverted, so should be consistent now.

Copy link
Collaborator

@shampson shampson left a comment

Choose a reason for hiding this comment

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

I added a couple comments. I think it makes sense for finish() to not close until everything is read out, but not necessarily for reset. Let me know your thoughts.

@aboba
Copy link
Collaborator Author

aboba commented Aug 15, 2018

Agree. Is the text consistent with that now?

index.html Outdated
<li> The QUIC stream has received STREAM frame with the FIN flag set.</li>
for transmission, the write buffer to be cleared and <var>stream</var>'s
<a>[[\Writeable]]</a> slot to be set to <code>false</code>.</li>
<li><var>stream</var> has <a>finished reading</a>.</li>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good 👍

@shampson shampson merged commit 4494f64 into master Aug 17, 2018
@shampson shampson deleted the issue-30-patch branch October 3, 2018 18:07
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.

2 participants