-
-
Notifications
You must be signed in to change notification settings - Fork 34.5k
lib: add support for readable byte streams to .toWeb() #58664
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
Changes from 1 commit
c5c4351
ca0aa34
b9e395a
814af3b
e9cca53
36050d8
385f3fc
1e82627
8f475bd
7efa8d2
ffcf924
8612c1e
f9e4a86
55af65e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Add support for the creation of ReadableByteStream to Readable.toWeb()
and Duplex.toWeb()
This enables the use of .getReader({ mode: "byob" }) on
e.g. socket().toWeb()
Refs: #56004 (comment)
Refs: https://developer.mozilla.org/en-US/docs/Web/API/Streams_API/Using_readable_byte_streams- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,70 @@ | ||||||||||||
| 'use strict'; | ||||||||||||
| const common = require('../common'); | ||||||||||||
| if (!common.hasCrypto) { common.skip('missing crypto'); } | ||||||||||||
|
|
||||||||||||
| const { Readable } = require('stream'); | ||||||||||||
| const process = require('process'); | ||||||||||||
| const { randomBytes } = require('crypto'); | ||||||||||||
| const assert = require('assert'); | ||||||||||||
|
|
||||||||||||
| // Based on: https://github.com/nodejs/node/issues/46347#issuecomment-1413886707 | ||||||||||||
| // edit: make it cross-platform as /dev/urandom is not available on Windows | ||||||||||||
| { | ||||||||||||
| let currentMemoryUsage = process.memoryUsage().arrayBuffers; | ||||||||||||
|
|
||||||||||||
| // We initialize a stream, but not start consuming it | ||||||||||||
| const randomNodeStream = new Readable({ | ||||||||||||
| read(size) { | ||||||||||||
| randomBytes(size, (err, buffer) => { | ||||||||||||
| if (err) { | ||||||||||||
| // If an error occurs, emit an 'error' event | ||||||||||||
| this.emit('error', err); | ||||||||||||
| return; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // Push the random bytes to the stream | ||||||||||||
| this.push(buffer); | ||||||||||||
| }); | ||||||||||||
| } | ||||||||||||
| }); | ||||||||||||
| // after 2 seconds, it'll get converted to web stream | ||||||||||||
| let randomWebStream; | ||||||||||||
|
|
||||||||||||
| // We check memory usage every second | ||||||||||||
| // since it's a stream, it shouldn't be higher than the chunk size | ||||||||||||
| const reportMemoryUsage = () => { | ||||||||||||
| const { arrayBuffers } = process.memoryUsage(); | ||||||||||||
| currentMemoryUsage = arrayBuffers; | ||||||||||||
|
|
||||||||||||
| assert(currentMemoryUsage <= 256 * 1024 * 1024); | ||||||||||||
| }; | ||||||||||||
| setInterval(reportMemoryUsage, 1000); | ||||||||||||
|
|
||||||||||||
| // after 1 second we use Readable.toWeb | ||||||||||||
| // memory usage should stay pretty much the same since it's still a stream | ||||||||||||
| setTimeout(() => { | ||||||||||||
| randomWebStream = Readable.toWeb(randomNodeStream, { type: 'bytes' }); | ||||||||||||
| }, 1000); | ||||||||||||
|
|
||||||||||||
| // after 2 seconds we start consuming the stream | ||||||||||||
| // memory usage will grow, but the old chunks should be garbage-collected pretty quickly | ||||||||||||
| setTimeout(async () => { | ||||||||||||
|
|
||||||||||||
| const reader = randomWebStream.getReader({ mode: 'byob' }); | ||||||||||||
|
|
||||||||||||
| let done = false; | ||||||||||||
| while (!done) { | ||||||||||||
| // Read a 16 bytes of data from the stream | ||||||||||||
| const result = await reader.read(new Uint8Array(16)); | ||||||||||||
| done = result.done; | ||||||||||||
| // We consume the stream, but we don't do anything with the data | ||||||||||||
| // This is to ensure that the stream is being consumed | ||||||||||||
| // and that the memory usage is being reported correctly | ||||||||||||
| } | ||||||||||||
| }, 2000); | ||||||||||||
|
|
||||||||||||
| setTimeout(() => { | ||||||||||||
| // Test considered passed if we don't crash | ||||||||||||
| process.exit(0); | ||||||||||||
| }, 5000); | ||||||||||||
|
||||||||||||
| setTimeout(() => { | |
| // Test considered passed if we don't crash | |
| process.exit(0); | |
| }, 5000); | |
| } |
Do you have any suggestions on how to code these tests without using timers?
Kind regards,
Hans
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that test/parallel/test-stream-readable-to-web.js is doing a rather crude memory leak test. Which indeed does not make sense to replicate for this bytestream case as the new code only modifies the parameters for the creation of a new ReadableStream and does not touch any buffers itself.
Therefore I replaced the memory leak test by a simple test to make sure that the bytestream works as intented. This test does not need timers at all.
Kind regards,
Hans
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| 'use strict'; | ||
| require('../common'); | ||
| const { Readable } = require('stream'); | ||
| { | ||
| const r = Readable.from([]); | ||
| // Cancelling reader while closing should not cause uncaught exceptions | ||
| r.on('close', () => reader.cancel()); | ||
jasnell marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| const reader = Readable.toWeb(r, { type: 'bytes' }).getReader({ mode: 'byob' }); | ||
| reader.read(new Uint8Array(16)); | ||
jasnell marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.