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: eos on closed #28748

Closed
wants to merge 12 commits into from
Closed

Conversation

ronag
Copy link
Member

@ronag ronag commented Jul 18, 2019

stream.finished should be invoked if stream is already closed/destroyed. Currently there is a potential deadlock depending on when stream.finished is called.

For writable premature close is close before finish, i.e. it should look for finished instead of ended.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@ronag ronag force-pushed the stream-eos-on-closed branch 8 times, most recently from 3c57be4 to c9d1e19 Compare July 22, 2019 06:42
@Trott
Copy link
Member

Trott commented Jul 23, 2019

/ping @nodejs/streams

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind to add the equivalent test for Readable as well?

lib/internal/streams/end-of-stream.js Outdated Show resolved Hide resolved
lib/internal/streams/end-of-stream.js Outdated Show resolved Hide resolved
test/parallel/test-stream-eos.js Outdated Show resolved Hide resolved
@ronag
Copy link
Member Author

ronag commented Jul 23, 2019

@mcollina updated

@ronag ronag force-pushed the stream-eos-on-closed branch 2 times, most recently from aefe068 to 1588c78 Compare July 23, 2019 07:37
lib/internal/streams/end-of-stream.js Outdated Show resolved Hide resolved
@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 23, 2019
@ronag ronag force-pushed the stream-eos-on-closed branch 3 times, most recently from 40b4c0b to 39ca23a Compare July 24, 2019 09:46
@Fishrock123 Fishrock123 added the stream Issues and PRs related to the stream subsystem. label Jul 24, 2019
@ronag
Copy link
Member Author

ronag commented Aug 2, 2019

@benjamingr ping

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina mcollina requested a review from a team August 3, 2019 16:53
@mcollina
Copy link
Member

mcollina commented Aug 3, 2019

@nodejs/tsc this needs another review.

@ronag
Copy link
Member Author

ronag commented Aug 6, 2019

@mcollina: Should this maybe result in a ERR_STREAM_DESTROYED?

@ronag ronag mentioned this pull request Aug 6, 2019
2 tasks
@ronag ronag mentioned this pull request Sep 1, 2019
4 tasks
@ronag
Copy link
Member Author

ronag commented Sep 1, 2019

Yes please.

@mcollina: done

@ronag
Copy link
Member Author

ronag commented Sep 1, 2019

@Trott: Updated PR description to describe the included changes from the other PR (which is now closed).

@ronag
Copy link
Member Author

ronag commented Sep 1, 2019

This needs another CITGM (once Travis is ok) given the recent changes.

return callback.call(stream, err);
callback.call(stream, new ERR_STREAM_PREMATURE_CLOSE());
} else if (writable && !writableFinished) {
callback.call(stream, new ERR_STREAM_PREMATURE_CLOSE());
}
Copy link
Member Author

@ronag ronag Sep 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an added test for this change. We should always be emitting premature close unless we've seen finish or end events. I don't think this actually changes anything in practice.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mcollina
Copy link
Member

mcollina commented Sep 2, 2019

This will need a rebase, otherwise it looks good to land.

@ronag
Copy link
Member Author

ronag commented Sep 2, 2019

rebased

@nodejs-github-bot
Copy link
Collaborator

@mcollina
Copy link
Member

mcollina commented Sep 3, 2019

Landed in b03845b

@mcollina mcollina closed this Sep 3, 2019
mcollina pushed a commit that referenced this pull request Sep 3, 2019
Make stream.finished callback invoked if stream is already
closed/destroyed.

PR-URL: #28748
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@mafintosh
Copy link
Member

A little late to the party here, but this includes a big change in behaviour for streams.

Since the finished flag is used instead of the ended flag, this PR changes the behaiviour of eos to the following:

const stream = require('stream')
const ws = new stream.Writable({
  write (data, enc, cb) {
    process.nextTick(cb, null)
  }
})

finished(ws, function (err) {
  console.log('This used to *not* error:', err)
})

ws.write('data')
ws.end()
ws.emit('close') // modules tend to emit close on resource close but in a non error state

I was just bitten by this in a debugging session, so unsure if that was an intentional change?
If so I think it should be clearly documented :)

Streams are hard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants