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

stream: fix end-of-stream for HTTP/2 #24926

Closed
wants to merge 1 commit into from

Commits on Dec 9, 2018

  1. stream: fix end-of-stream for HTTP/2

    HTTP/2 streams call `.end()` on themselves from their
    `.destroy()` method, which might be queued (e.g. due to network
    congestion) and not processed before the stream itself is destroyed.
    
    In that case, the `_writableState.ended` property could be set before
    the stream emits its `'close'` event, and never actually emits the
    `'finished'` event, confusing the end-of-stream implementation so
    that it wouldn’t call its callback.
    
    This can be fixed by watching for the end events themselves using the
    existing `'finish'` and `'end'` listeners rather than relying on the
    `.ended` properties of the `_...State` objects.
    
    These properties still need to be checked to know whether stream
    closure was premature – My understanding is that ideally, streams
    should not emit `'close'` before `'end'` and/or `'finished'`, so this
    might be another bug, but changing this would require modifying tests
    and almost certainly be a breaking change.
    
    Fixes: nodejs#24456
    addaleax committed Dec 9, 2018
    Configuration menu
    Copy the full SHA
    379c878 View commit details
    Browse the repository at this point in the history