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: correct emit error in onConnect, full tests #15080

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@apapirovski
Member

apapirovski commented Aug 29, 2017

Minor correction in requestOnConnect to emit session error on NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE rather than stream error. Mostly this is about adding a full test suite for the error handling within requestOnConnect.

The test cases are a bit complicated as I felt like it was important to actually test all available errors, in case someone ever goes in and expands the switch statement to handle any other specific errors (or removes one of the currently handled ones). This way it avoids any accidental regressions where errors start emitting somewhere different than expected.

I also have a version of this for pushStream that I'll adjust based on feedback here and then create a PR. (It's also emitting stream error on NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE.)

(I tried to find if there was any convention for mocking functions but it seemed like this is how it was done elsewhere.)

Let me know if there's anything I can adjust. Thanks!

Refs: #14985

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

http2, test

@jasnell jasnell requested a review from mcollina Aug 29, 2017

http2: correct emit error in onConnect, full tests
Correct requestOnConnect to emit session error on
NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE rather than stream error. Add full
test suite for the error handling within requestOnConnect.

Refs: #14985
@mcollina

This comment has been minimized.

Member

mcollina commented Aug 30, 2017

This change is ok.

Can you remove nextTick from all the case in requestOnConnect, and rather wrap where it is called with a nextTick:
https://github.com/apapirovski/node/blob/89ea3984503b75284839016d7f818e7861f44977/lib/internal/http2/core.js#L1148?

I think it would be easier to debug in this way.

@apapirovski

This comment has been minimized.

Member

apapirovski commented Aug 30, 2017

It looks like right now only the event emits (in fact, looks like only the errors) are being run on nextTick, whereas a lot of the other stuff might be sync? So I guess I just wanted to confirm that we're ok with moving this up to where requestOnConnect is called? Thanks.

@mcollina

This comment has been minimized.

Member

mcollina commented Aug 30, 2017

@apapirovski maybe not - what do you think @jasnell?

@apapirovski

This comment has been minimized.

Member

apapirovski commented Sep 1, 2017

@jasnell & @mcollina anything I can do to get this approved? Thanks!

@jasnell

jasnell approved these changes Sep 1, 2017

LGTM with green CI!

Thank you!

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 1, 2017

@mcollina ... I think I'm ok with this but would like your opinion on it as well. If you're not convinced we can talk it through and figure out what's better

@mcollina

LGTM

@mcollina

This comment has been minimized.

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Sep 3, 2017

Landed in 8d5b013

@BridgeAR BridgeAR closed this Sep 3, 2017

BridgeAR added a commit that referenced this pull request Sep 3, 2017

http2: correct emit error in onConnect, full tests
Correct requestOnConnect to emit session error on
NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE rather than stream error. Add full
test suite for the error handling within requestOnConnect.

PR-URL: #15080
Refs: #14985
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

@apapirovski apapirovski deleted the apapirovski:patch-http2-fix-and-test-request-on-connect branch Sep 4, 2017

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 5, 2017

http2: correct emit error in onConnect, full tests
Correct requestOnConnect to emit session error on
NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE rather than stream error. Add full
test suite for the error handling within requestOnConnect.

PR-URL: nodejs/node#15080
Refs: nodejs/node#14985
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 10, 2017

http2: correct emit error in onConnect, full tests
Correct requestOnConnect to emit session error on
NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE rather than stream error. Add full
test suite for the error handling within requestOnConnect.

PR-URL: #15080
Refs: #14985
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 10, 2017

Merged

v8.5.0 proposal #15308

MylesBorins added a commit that referenced this pull request Sep 11, 2017

http2: correct emit error in onConnect, full tests
Correct requestOnConnect to emit session error on
NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE rather than stream error. Add full
test suite for the error handling within requestOnConnect.

PR-URL: #15080
Refs: #14985
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 12, 2017

http2: correct emit error in onConnect, full tests
Correct requestOnConnect to emit session error on
NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE rather than stream error. Add full
test suite for the error handling within requestOnConnect.

PR-URL: #15080
Refs: #14985
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment