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

streamError never emitted #20211

Closed
ronag opened this issue Apr 22, 2018 · 8 comments
Closed

streamError never emitted #20211

ronag opened this issue Apr 22, 2018 · 8 comments
Labels
confirmed-bug Issues with confirmed bugs. http2 Issues or PRs related to the http2 subsystem.

Comments

@ronag
Copy link
Member

ronag commented Apr 22, 2018

Reading the http/2 compat code it mentions a streamError on the server object.

function onStreamError(error) {
  // this is purposefully left blank
  //
  // errors in compatibility mode are
  // not forwarded to the request
  // and response objects. However,
  // they are forwarded to 'streamError'
  // on the server by Http2Stream
}

However, this doesn't seem to be emitted anywhere in the code? Is this something that has been removed from core http/2 and has a leftovers in compat and docs?

@Trott
Copy link
Member

Trott commented Apr 23, 2018

@nodejs/http2

@Trott Trott added the http2 Issues or PRs related to the http2 subsystem. label Apr 23, 2018
@ryzokuken
Copy link
Contributor

You're right, streamError isn't emitted anywhere in the source code. Adding the "confirmed" label.

@ryzokuken ryzokuken added the confirmed-bug Issues with confirmed bugs. label Apr 23, 2018
@ryzokuken
Copy link
Contributor

ryzokuken commented Apr 23, 2018

The docs say:

#### Event: 'streamError'
<!-- YAML
added: v8.5.0
-->

If a `ServerHttp2Stream` emits an `'error'` event, it will be forwarded here.
The stream will already be destroyed when this event is triggered.

@ryzokuken
Copy link
Contributor

ryzokuken commented Apr 23, 2018

@mcollina @mafintosh this could be resolved by attatching a listener onto the corresponding ServerHttp2Stream that'd emit streamError on the current instance, right?

@apapirovski
Copy link
Member

streamError was removed at some point and the documentation + the code comments are just lagging behind.

@mcollina
Copy link
Member

All of this is buggy and needs to be refactored. However, there are multiple intiative/prs going in parallel. There is also an old PR from myself that was reverted that fixed some of this, and I forgot to resubmit.

Can you please leave it alone for some weeks? I’m currently on vacation and with all the rush of Node 10 I had no time to write all of this down.

streamError is part of the HTTP1 compatibility mode, and it’s a good destination. It should not be removed.

@apapirovski
Copy link
Member

apapirovski commented Apr 24, 2018

@mcollina I don't think streamError exists in the current codebase and I've gone through it several times. I believe it was removed in 0babd18

ping @jasnell to ascertain whether that was intentional or not

@mcollina
Copy link
Member

This should be fixed first: #19852. It fixes the problems introduced by that PR. It’s not done yet and it can use some help.

I am on vacation for next week or so with limited connectivity.

jasnell added a commit to jasnell/node that referenced this issue Aug 10, 2018
`streamError` was removed quite some time ago but the docs and
code comments weren't updated. Fix that.

Fixes: nodejs#20211
targos pushed a commit that referenced this issue Aug 12, 2018
`streamError` was removed quite some time ago but the docs and
code comments weren't updated. Fix that.

Fixes: #20211

PR-URL: #22246
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
kjin pushed a commit to kjin/node that referenced this issue Sep 25, 2018
`streamError` was removed quite some time ago but the docs and
code comments weren't updated. Fix that.

Fixes: nodejs#20211

PR-URL: nodejs#22246
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
kjin pushed a commit to kjin/node that referenced this issue Oct 16, 2018
`streamError` was removed quite some time ago but the docs and
code comments weren't updated. Fix that.

Fixes: nodejs#20211

PR-URL: nodejs#22246
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BethGriggs pushed a commit that referenced this issue Oct 17, 2018
`streamError` was removed quite some time ago but the docs and
code comments weren't updated. Fix that.

Fixes: #20211

Backport-PR-URL: #22850
PR-URL: #22246
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants