Bump readable-stream from 3.6.0 to 4.0.0 #3

dependabot · 2022-07-01T16:09:41Z

Bumps readable-stream from 3.6.0 to 4.0.0.

Release notes

Sourced from readable-stream's releases.

v4.0.0

What's Changed

Add ronag to the streams wg by @ronag in nodejs/readable-stream#430

Add vweevers to streams wg by @vweevers in nodejs/readable-stream#431

Use the shared Travis config by @dominykas in nodejs/readable-stream#444

tests: add GitHub Actions CI by @bnb in nodejs/readable-stream#418

Upgrade airtap and use GitHub Actions by @vweevers in nodejs/readable-stream#443

Adds Node v14 to github actions by @mcollina in nodejs/readable-stream#445

Cleanup CI by @vweevers in nodejs/readable-stream#446

Add usage instructions for Webpack 5 by @vweevers in nodejs/readable-stream#455

Update to Node 18.0.0 by @ShogunPanda in nodejs/readable-stream#471

Evade override mistake by @erights in nodejs/readable-stream#474

v4 launch details by @mcollina in nodejs/readable-stream#472

New Contributors

@ronag made their first contribution in nodejs/readable-stream#430

@dominykas made their first contribution in nodejs/readable-stream#444

@bnb made their first contribution in nodejs/readable-stream#418

@ShogunPanda made their first contribution in nodejs/readable-stream#471

@erights made their first contribution in nodejs/readable-stream#474

Full Changelog: nodejs/readable-stream@v3.6.0...v4.0.0

Commits

49a45a9 Bumped v4.0.0
37935df v4 launch details (#472)
431a9af Evade override mistake (#474)
81c5264 Update to Node 18.0.0 (#471)
97fd94f doc: make references to primary branch be main
bcbe5e0 Add usage instructions for Webpack 5 (#455)
07462e1 Cleanup CI (#446)
8c73012 Adds Node v14 to github actions (#445)
19381e9 Upgrade airtap and use GitHub Actions (#443)
69aa464 tests: add GitHub Actions CI (#418)
Additional commits viewable in compare view

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.

Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

@dependabot rebase will rebase this PR
@dependabot recreate will recreate this PR, overwriting any edits that have been made to it
@dependabot merge will merge this PR after your CI passes on it
@dependabot squash and merge will squash and merge this PR after your CI passes on it
@dependabot cancel merge will cancel a previously requested merge and block automerging
@dependabot reopen will reopen this PR if it is closed
@dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
@dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

Bumps [readable-stream](https://github.com/nodejs/readable-stream) from 3.6.0 to 4.0.0. - [Release notes](https://github.com/nodejs/readable-stream/releases) - [Commits](nodejs/readable-stream@v3.6.0...v4.0.0) --- updated-dependencies: - dependency-name: readable-stream dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>

vweevers · 2022-07-02T13:36:56Z

In addition to the Node.js 12 test failure (Level/many-level#7 (comment)) which I intend to "fix" by dropping support of Node.js 12, there are also random failures in the multi-process.js test, which unfortunately also happens on other Node.js versions. So far I'm unable to reproduce it with readable-stream v3, only v4.

# multiple processes
ok 38 should be deeply equivalent
not ok 39 should be deeply equivalent
  ---
    operator: deepEqual
    expected: |-
      [ [ '00000', '0.6959290688256938' ], [ '00001', '0.09414721663549419' ], [ '00002', '0.7016490817431085' ], [ '00003', '0.04483456956106591' ], [ '00004', '0.9068256992630355' ], [ '00005', '0.2594698226076928' ], [ '00006', '0.6541768373887236' ], [ '00007', '0.7025560544834328' ], [ '00008', '0.6187830617601537' ], [ '00009', '0.0394596046102651' ] ]
    actual: |-
      [ [ '00001', '0.09414721663549419' ], [ '00002', '0.7016490817431085' ], [ '00003', '0.04483456956106591' ], [ '00005', '0.2594698226076928' ], [ '00008', '0.6187830617601537' ], [ '00009', '0.0394596046102651' ] ]

vweevers · 2022-07-02T14:24:54Z

It reproduces more consistently with v4, across all node versions and operating systems: https://github.com/Level/rave-level/actions/runs/2601809114. But it also reproduces with v3 on mac: https://github.com/Level/rave-level/actions/runs/2601819174. And also with many-level@1: https://github.com/Level/rave-level/actions/runs/2601834432

In short, this is a pre-existing bug.

vweevers · 2022-07-02T18:09:37Z

To summarize (and correct) the above, there are 3 separate issues here:

pipeline() of readable-stream@4 on Node.js 12: Premature close error on socket under Node.js 12 nodejs/readable-stream#477
pipeline() of readable-stream@4 on any Node.js version: calls its callback before the streams have emitted 'close', which then leads to side effects
EADDRINUSE errors on Mac, which I now see are distinct from the other cases.

1 and 2 can be fixed by replacing pipeline() with a simpler, stricter function that always waits for 'close' events, instead of guessing which streams will emit 'close'.

As for 3, I have no idea. We can tackle that separately though, because it's unrelated to readable-stream.

juliangruber · 2022-07-02T18:12:12Z

Can we just drop Node 12 here?
Instead of relying on "close" having been emitted, could we adjust the tests to work before that event?

vweevers · 2022-07-02T18:48:40Z

Can we just drop Node 12 here?

Yeah, we could.

Instead of relying on "close" having been emitted, could we adjust the tests to work before that event?

The issue isn't in the tests. It's a general problem: before 'close' has been emitted, resources haven't been released yet. This problem pops up in scenarios with high contention, like on the LevelDB lock and socket path here. Node core's pipeline() wants to avoid the problem but it can't, because not all ecosystem modules are guaranteed to emit 'close'. In rave-level, all relevant streams are ours (and internal) so we do have that guarantee.

vweevers · 2022-07-02T20:42:08Z

You did give me pause though, and made me look at my test code (with a pipeline replacement) again. I found a potential bug there, leading me to wrong conclusions. Will update.

vweevers · 2022-07-03T21:38:30Z

Waiting for 'close' events (or just doing a setTimeout()) merely reduces the chance of it happening. The actual cause is that pipeline(socket, this.createRpcStream({ ref: socket }), socket, cb) sometimes doesn't call cb and leaves the rpc stream in a half-open state (writable side finishes but readable side does not).

This can be seen in the following repro, which also contains a fix that we could make in many-level:

const { pipeline, Duplex, PassThrough, finished } = require('readable-stream')
const net = require('net')
const cp = require('child_process')
const socketPath = process.platform === 'win32' ? '\\\\.\\pipe\\stream-test' : __dirname + '/stream-test.sock'

if (process.argv[2] === 'server') {
  const server = net.createServer(sock => {
    sock.pipe(new PassThrough()).pipe(sock)
  })

  server.listen(socketPath, function () {
    process.send('ready')
  })
} else {
  const child = cp.fork(__filename, ['server'], { timeout: 2e3 })

  child.once('message', function ready () {
    const socket = net.connect(socketPath)
    const encode = new PassThrough()
    const decode = new PassThrough()
    const proxy = Duplex.from({ writable: decode, readable: encode })

    decode.on('data', function (x) {
      console.log('received %s', x)
    })

    // (1) Notice how the encode stream does not end, finish or close
    for (const event of ['close', 'finish', 'end']) {
      socket.on(event, () => console.log('socket', event))
      proxy.on(event, () => console.log('proxy', event))
      encode.on(event, () => console.log('encode', event))
      decode.on(event, () => console.log('decode', event))
    }

    pipeline(socket, proxy, socket, function () {
      // (2) So we never get here
      console.log('done')
    })

    encode.write('x')

    // (3) To fix, uncomment:
    // finished(proxy, { writable: true, readable: false }, () => encode.destroy())
  })
}

juliangruber · 2022-07-04T06:37:33Z

Ok, great that you were able to reproduce this using core libs only! Do you think it's an issue there or adding finished is actually the way to go?

vweevers · 2022-07-04T08:31:20Z

I hope to further simplify the repro (removing net, child_process and the amount of duplex streams) to then open an issue in nodejs. Because it feels like a bug, but I can't yet clearly articulate how I expect pipeline() to behave. It's an odd case because net does not emit an error on connection loss of a unix socket or named pipe, unlike on tcp sockets.

juliangruber · 2022-07-04T11:44:48Z

Cool, I'm interested in this and will take a stab at simplifying it

juliangruber · 2022-07-04T12:05:45Z

I'm seeing the same behavior with tcp sockets, in codespaces

juliangruber · 2022-07-04T12:12:00Z

Example without child processes:

const { pipeline, Duplex, PassThrough, finished } = require('readable-stream')
const net = require('net')

const cons = []
const server = net.createServer(sock => {
  cons.push(sock)
  sock.pipe(new PassThrough()).pipe(sock)
})

server.listen(8000, function () {
  const socket = net.connect(8000)
  const encode = new PassThrough()
  const decode = new PassThrough()
  const proxy = Duplex.from({ writable: decode, readable: encode })

  decode.on('data', function (x) {
    console.log('received %s', x)
  })

  // (1) Notice how the encode stream does not end, finish or close
  for (const event of ['close', 'finish', 'end']) {
    socket.on(event, () => console.log('socket', event))
    proxy.on(event, () => console.log('proxy', event))
    encode.on(event, () => console.log('encode', event))
    decode.on(event, () => console.log('decode', event))
  }

  pipeline(socket, proxy, socket, function () {
    // (2) So we never get here
    console.log('done')
  })

  encode.write('x')

  // (3) To fix, uncomment:
  // finished(proxy, { writable: true, readable: false }, () => encode.destroy())

  setTimeout(() => {
    server.close()
    cons[0].end()
  }, 2000)
})

juliangruber · 2022-07-04T12:13:18Z

Also removed net code:

const { pipeline, Duplex, PassThrough, finished } = require('readable-stream')

const socket = new PassThrough()
const encode = new PassThrough()
const decode = new PassThrough()
const proxy = Duplex.from({ writable: decode, readable: encode })

decode.on('data', function (x) {
  console.log('received %s', x)
})

// (1) Notice how the encode stream does not end, finish or close
for (const event of ['close', 'finish', 'end']) {
  socket.on(event, () => console.log('socket', event))
  proxy.on(event, () => console.log('proxy', event))
  encode.on(event, () => console.log('encode', event))
  decode.on(event, () => console.log('decode', event))
}

pipeline(socket, proxy, socket, function () {
  // (2) So we never get here
  console.log('done')
})

encode.write('x')

// (3) To fix, uncomment:
// finished(proxy, { writable: true, readable: false }, () => encode.destroy())

setTimeout(() => {
  socket.end()
}, 2000)

juliangruber · 2022-07-04T12:16:45Z

While it is reproducible like this, I'm not sure it still shows the same issue, because we're not ending the encode stream, so it looks more like a design issue here, where we need to start manually propagating stream events

vweevers · 2022-07-04T16:34:10Z

If you do socket.destroy() on the last example then it does get to console.log('done'), due to a Premature close error that triggers everything to be destroyed. What we need here is to get that same behavior if the socket closed without an error, because either way, the streams are now unusable.

The writable side of our proxy stream behaves as I expect. The readable side - which is connected to a closed socket stream - does not. If we did pipeline(x, proxy), i.e. proxy being the last stream, then I'd understand it, because the pipeline isn't consuming the readable side of the proxy, and you can chain pipelines this way.

But in pipeline(x, proxy, x) I want the pipeline to fully manage the lifetime of the proxy.

mcollina · 2022-07-04T16:48:00Z

Is there a bug in Node.js core v18 for this? There might be. If you have a simpler repo just open an issue and we can discuss it there.

vweevers · 2022-07-04T17:58:34Z

Further simplified. Reproduces on 18.4.0 core streams as well. I'll test it on some more versions and then open an issue.

const { pipeline, Duplex, PassThrough, finished } = require('readable-stream')

const remote = new PassThrough()
const local = new Duplex({
  read () {},
  write (chunk, enc, callback) {
    callback()
  }
})

for (const event of ['close', 'finish', 'end']) {
  remote.on(event, () => console.log('remote', event))
  local.on(event, () => console.log('local', event))
}

pipeline(remote, local, remote, function () {
  // We never get here
  console.log('done')
})

// To fix, uncomment:
// finished(local, { writable: true, readable: false }, () => local.destroy())

setImmediate(() => {
  remote.end()
})

mcollina · 2022-07-04T17:59:25Z

Thx!

vweevers · 2022-07-04T18:44:20Z

nodejs/node#43682

dependabot · 2022-08-01T16:08:54Z

Superseded by #6.

dependabot bot added dependencies Pull requests that update a dependency file javascript Pull requests that update Javascript code labels Jul 1, 2022

vweevers mentioned this pull request Jul 2, 2022

Replace duplexify and friends with readable-stream v4 Level/many-level#7

Merged

This was referenced Jul 10, 2022

Fix random test failures on Mac #4

Merged

Bump many-level from 1.0.1 to 2.0.0 #5

Open

dependabot bot closed this Aug 1, 2022

dependabot bot deleted the dependabot/npm_and_yarn/readable-stream-4.0.0 branch August 1, 2022 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump readable-stream from 3.6.0 to 4.0.0 #3

Bump readable-stream from 3.6.0 to 4.0.0 #3

dependabot bot commented on behalf of github Jul 1, 2022

vweevers commented Jul 2, 2022 •

edited

vweevers commented Jul 2, 2022 •

edited

vweevers commented Jul 2, 2022

juliangruber commented Jul 2, 2022

vweevers commented Jul 2, 2022 •

edited

vweevers commented Jul 2, 2022

vweevers commented Jul 3, 2022 •

edited

juliangruber commented Jul 4, 2022

vweevers commented Jul 4, 2022

juliangruber commented Jul 4, 2022

juliangruber commented Jul 4, 2022

juliangruber commented Jul 4, 2022

juliangruber commented Jul 4, 2022

juliangruber commented Jul 4, 2022

vweevers commented Jul 4, 2022

mcollina commented Jul 4, 2022

vweevers commented Jul 4, 2022

mcollina commented Jul 4, 2022

vweevers commented Jul 4, 2022

dependabot bot commented on behalf of github Aug 1, 2022

Bump readable-stream from 3.6.0 to 4.0.0 #3

Bump readable-stream from 3.6.0 to 4.0.0 #3

Conversation

dependabot bot commented on behalf of github Jul 1, 2022

v4.0.0

What's Changed

New Contributors

vweevers commented Jul 2, 2022 • edited

vweevers commented Jul 2, 2022 • edited

vweevers commented Jul 2, 2022

juliangruber commented Jul 2, 2022

vweevers commented Jul 2, 2022 • edited

vweevers commented Jul 2, 2022

vweevers commented Jul 3, 2022 • edited

juliangruber commented Jul 4, 2022

vweevers commented Jul 4, 2022

juliangruber commented Jul 4, 2022

juliangruber commented Jul 4, 2022

juliangruber commented Jul 4, 2022

juliangruber commented Jul 4, 2022

juliangruber commented Jul 4, 2022

vweevers commented Jul 4, 2022

mcollina commented Jul 4, 2022

vweevers commented Jul 4, 2022

mcollina commented Jul 4, 2022

vweevers commented Jul 4, 2022

dependabot bot commented on behalf of github Aug 1, 2022

vweevers commented Jul 2, 2022 •

edited

vweevers commented Jul 2, 2022 •

edited

vweevers commented Jul 2, 2022 •

edited

vweevers commented Jul 3, 2022 •

edited