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

change readStop to private function #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hotyhuang
Copy link

@hotyhuang hotyhuang commented Jun 11, 2020

To fix issue: spdy-http2/node-spdy#369
Issue reproduce POC: https://github.com/hotyhuang/SSEexample

I found out that this issue with SSE is caused by recent commit in Nodejs: nodejs/node@138eb32
I personally is not familiar with spdy code, just tried out this solution, and looks like it's working perfectly with node v10 ~ v12.

@aphix
Copy link

aphix commented Sep 18, 2020

Friendly ping: @indutny @daviddias

Cheers =)

@aphix
Copy link

aphix commented Sep 18, 2020

I'm still able to repro the issue (spdy-http2/node-spdy#369) after applying this patch locally, will put together a repro and work on a fix this weekend if not later today. @hotyhuang

FWIW, I didn't try your repro repo, but was able to get it to happen in 12.18.3 by spamming ctrl+shift+r in Firefox with a listening server.

I believe this is a graph of what happened (I did capture a profile, but I can't share it publicly, so let me know if there's a private way to pass it along to one of the maintainers):
image

As far as I've tracked it thus far, following a RST, this callback in spdy-transport:

https://github.com/spdy-http2/spdy-transport/blob/master/lib/spdy-transport/stream.js#L139

Is calling its callback twice in the _split function:

https://github.com/spdy-http2/spdy-transport/blob/master/lib/spdy-transport/stream.js#L237

And everything tears itself down afterward.

Also note: I'm NOT using req.push anywhere.

@hotyhuang
Copy link
Author

@aphix Thank you for paying attention to this issue. This is occurred long back when node version was v12.18.1. I am not sure if the updates changed anything.

By the time I was looking into this code, there seems a lot more logics behind the scenes, rather than the twice callback. That's why I changed the stop func to be private, to get rid of duplicate call by other internal methods.

If you could figure this out it will be great help! But i would rather expect some response from the owner of this repo (since they will know how to fix it properly, and thanks for pinning their ids).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants