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: stream.respondWithFile, although capturing stream error when file not found, it also reflects error to req and res in compat layer #14963

Closed
akc42 opened this issue Aug 21, 2017 · 2 comments
Labels
http2 Issues or PRs related to the http2 subsystem.

Comments

@akc42
Copy link

akc42 commented Aug 21, 2017

I'm using the http2.createSecureServer with a request handler which passes request (as req) and response (as res) streams to the handler.

I am using res.stream.respondWithFile()

I am trying to catch errors where the file is not found, so I listen for error event on the stream so my code is like below

      res.stream.on('error', err => {
        if (forwardError || !(err.code === 'ENOENT')) {
          next(err);
        } else {
          //this was probably file not found, in which case we just go to next middleware function.
          next();
        }
      });

      res.stream.respondWithFile(
        filename,
        { 'content-type': map[ext] || 'text/plain',
          'cache-control': 'no-cache' },
        { statCheck });

Even though I am handling the stream error, I am getting the same error reflected again on both req and res variables.

This is fundamentally the following issue with a slight twist (listening to the stream error)
nodejs/http2#168
which was raised before the code landed in node and is is still open

@addaleax
Copy link
Member

@nodejs/http2

@addaleax addaleax added the http2 Issues or PRs related to the http2 subsystem. label Aug 21, 2017
@jasnell
Copy link
Member

jasnell commented Aug 21, 2017

Yep, the error handling on the compat api side has not yet been updated. I'm hoping to get to it this week if someone else doesn't beat me to it

Trott pushed a commit to Trott/io.js that referenced this issue Aug 25, 2017
This changes the error handling model of ServerHttp2Stream,
ServerHttp2Request and ServerHttp2Response.
An 'error' emitted on ServerHttp2Stream will not go to
'uncaughtException' anymore, but to the server 'streamError'.
On the stream 'error', ServerHttp2Request will emit 'abort', while
ServerHttp2Response would do nothing

See: nodejs#14963

PR-URL: nodejs#14991
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
mcollina added a commit to mcollina/node that referenced this issue Aug 27, 2017
This changes the error handling model of ServerHttp2Stream,
ServerHttp2Request and ServerHttp2Response.
An 'error' emitted on ServerHttp2Stream will not go to
'uncaughtException' anymore, but to the server 'streamError'.
On the stream 'error', ServerHttp2Request will emit 'abort', while
ServerHttp2Response would do nothing

See: nodejs#14963
mcollina added a commit to mcollina/node that referenced this issue Aug 27, 2017
ghost pushed a commit to ayojs/ayo that referenced this issue Aug 30, 2017
This changes the error handling model of ServerHttp2Stream,
ServerHttp2Request and ServerHttp2Response.
An 'error' emitted on ServerHttp2Stream will not go to
'uncaughtException' anymore, but to the server 'streamError'.
On the stream 'error', ServerHttp2Request will emit 'abort', while
ServerHttp2Response would do nothing

See: nodejs/node#14963

PR-URL: nodejs/node#14991
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
ghost pushed a commit to ayojs/ayo that referenced this issue Aug 30, 2017
This changes the error handling model of ServerHttp2Stream,
ServerHttp2Request and ServerHttp2Response.
An 'error' emitted on ServerHttp2Stream will not go to
'uncaughtException' anymore, but to the server 'streamError'.
On the stream 'error', ServerHttp2Request will emit 'abort', while
ServerHttp2Response would do nothing.
It also updates respondWith* to the new error handling.

Fixes: nodejs/node#14963
PR-URL: nodejs/node#14991
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
ghost pushed a commit to ayojs/ayo that referenced this issue Aug 30, 2017
This changes the error handling model of ServerHttp2Stream,
ServerHttp2Request and ServerHttp2Response.
An 'error' emitted on ServerHttp2Stream will not go to
'uncaughtException' anymore, but to the server 'streamError'.
On the stream 'error', ServerHttp2Request will emit 'abort', while
ServerHttp2Response would do nothing

See: nodejs/node#14963

PR-URL: nodejs/node#14991
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
ghost pushed a commit to ayojs/ayo that referenced this issue Aug 30, 2017
This changes the error handling model of ServerHttp2Stream,
ServerHttp2Request and ServerHttp2Response.
An 'error' emitted on ServerHttp2Stream will not go to
'uncaughtException' anymore, but to the server 'streamError'.
On the stream 'error', ServerHttp2Request will emit 'abort', while
ServerHttp2Response would do nothing.
It also updates respondWith* to the new error handling.

Fixes: nodejs/node#14963
PR-URL: nodejs/node#14991
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins pushed a commit that referenced this issue Sep 10, 2017
This changes the error handling model of ServerHttp2Stream,
ServerHttp2Request and ServerHttp2Response.
An 'error' emitted on ServerHttp2Stream will not go to
'uncaughtException' anymore, but to the server 'streamError'.
On the stream 'error', ServerHttp2Request will emit 'abort', while
ServerHttp2Response would do nothing.
It also updates respondWith* to the new error handling.

Fixes: #14963
PR-URL: #14991
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins pushed a commit that referenced this issue Sep 12, 2017
This changes the error handling model of ServerHttp2Stream,
ServerHttp2Request and ServerHttp2Response.
An 'error' emitted on ServerHttp2Stream will not go to
'uncaughtException' anymore, but to the server 'streamError'.
On the stream 'error', ServerHttp2Request will emit 'abort', while
ServerHttp2Response would do nothing.
It also updates respondWith* to the new error handling.

Fixes: #14963
PR-URL: #14991
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
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

No branches or pull requests

3 participants