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

http2: close not emitted when receiving RST_STREAM after DATA #27863

Closed
mildsunrise opened this issue May 25, 2019 · 4 comments
Closed

http2: close not emitted when receiving RST_STREAM after DATA #27863

mildsunrise opened this issue May 25, 2019 · 4 comments
Labels
http2 Issues or PRs related to the http2 subsystem.

Comments

@mildsunrise
Copy link
Member

mildsunrise commented May 25, 2019

  • Version: latest master
  • Platform: Linux 4.4.0-142-generic 168-Ubuntu SMP Wed Jan 16 2019 x86_64 GNU/Linux
  • Subsystem: http2
const server = http2.createServer();
server.on('stream', (stream) => {
  stream.respond();
  stream.write('test');
  // Wait for DATA to be sent, then send RST_STREAM
  setTimeout(() => stream.close(), 200);
});

server.listen(() => {
  const client = http2.connect({ protocol: 'http:', ...server.address() });
  const req = client.request({ ':path': '/' });
  req.end();
  req.on('close', () => {
    // Never called
  });
});

I can verify that RST_STREAM is sent to the client, but close is never emitted. Is this expected?
If the write is removed, close is emitted.

@lpinca lpinca added the http2 Issues or PRs related to the http2 subsystem. label May 25, 2019
@mildsunrise
Copy link
Member Author

I have bisected this to #17406, to the second commit.
Relevant diffs: lib/internal/http2/core.js and src/node_http2.cc

@mildsunrise
Copy link
Member Author

mildsunrise commented May 26, 2019

...I feel a bit stupid. After #17406, the stream isn't destroyed immediately upon receiving an RST_STREAM, it's destroyed after all data has been consumed. Hence, doing req.resume() on the above example makes it emit the close event.

I think we should at least clarify the docs. Also, test-http2-large-write-destroy.js should be corrected to add req.resume(), unless I'm missing something.

@lpinca
Copy link
Member

lpinca commented May 26, 2019

Also, test-http2-large-write-destroy.js should be corrected to add req.resume(), unless I'm missing something.

Yes, seems correct to me.

mildsunrise added a commit to mildsunrise/node that referenced this issue May 26, 2019
Correct docs to clarify that behaviour,
and fix a race condition in test-http2-large-write-destroy.js.

Fixes: nodejs#27863
Trott pushed a commit to Trott/io.js that referenced this issue May 28, 2019
Correct docs to clarify that behaviour,
and fix a race condition in test-http2-large-write-destroy.js.

Fixes: nodejs#27863
PR-URL: nodejs#27891
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@Trott
Copy link
Member

Trott commented May 28, 2019

Fixed in 42d8011

@Trott Trott closed this as completed May 28, 2019
targos pushed a commit that referenced this issue May 31, 2019
Correct docs to clarify that behaviour,
and fix a race condition in test-http2-large-write-destroy.js.

Fixes: #27863
PR-URL: #27891
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
mildsunrise added a commit to mildsunrise/node that referenced this issue Jul 30, 2019
Correct docs to clarify that behaviour,
and fix a race condition in test-http2-large-write-destroy.js.

Fixes: nodejs#27863
PR-URL: nodejs#27891
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
BethGriggs pushed a commit that referenced this issue Oct 7, 2019
Correct docs to clarify that behaviour,
and fix a race condition in test-http2-large-write-destroy.js.

Fixes: #27863

Backport-PR-URL: #28904
PR-URL: #27891
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants