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

Add stream example #34

Closed
wants to merge 4 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 11 additions & 9 deletions examples/stream/port_duplex.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,35 +4,37 @@ const { Duplex } = require('stream');

class PortDuplex extends Duplex {
#port = undefined;
#transfer = false;

constructor (port, options) {
const {
readable = true,
writable = true,
transfer = false
writable = true
} = { ...options };
if (typeof readable !== 'boolean') {
throw new TypeError('readable must be a boolean');
}
if (typeof writable !== 'boolean') {
throw new TypeError('writable must be a boolean');
}
if (typeof transfer !== 'boolean') {
throw new TypeError('transfer must be a boolean');
}
super({ autoDestroy: true, readable, writable });
this.#port = port;
this.#transfer = transfer;
this.#port.onmessage = PortDuplex.#onmessage.bind(this);
}

_write (chunk, encoding, callback) {
if (typeof chunk === 'string') {
chunk = Buffer.from(chunk, encoding);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will always be a buffer, you are not allowing the encoding to be changed.

const transferList = this.#transfer ? [chunk.buffer] : undefined;
this.#port.postMessage(chunk, transferList);
// Be sure to always copy the chunk here and never use a
// transferList. There are several reasons:
// a) Buffer instances are most often created off a pool
// and share the same underlying common ArrayBuffer,
// transferring those can break Node.js in many ways.
Copy link
Collaborator

Choose a reason for hiding this comment

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

See the comment in the Node.js thread, this is true but not necessarily a concern in the future. The sceond concern is definitely there, though :)

// b) The Buffer instance may still be used by some
// other upstream component. Transferring it here
// will cause unexpected and undefined behavior that
// will likely crash the Node.js process.
this.#port.postMessage(chunk);
callback();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a feeling there should be some sort of backpressure built in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Absolutely it should

}

Expand Down