Add stream example #34

jasnell · 2020-05-04T20:57:27Z

@addaleax @mcollina ... here's a super simple example of how streams can be supported.

@addaleax ... interestingly, this example hits an Abort inside Node.js when we replace createBrotliCompress with createGzip or createDeflate within the worker.js...

james@ubuntu:~/nearform/piscina/examples/stream$ node index
node[30456]: ../src/node_zlib.cc:323:static void node::{anonymous}::CompressionStream<CompressionContext>::Write(const v8::FunctionCallbackInfo<v8::Value>&) [with bool async = true; CompressionContext = node::{anonymous}::ZlibContext]: Assertion `Buffer::IsWithinBounds(out_off, out_len, Buffer::Length(out_buf))' failed.
 1: 0xa295b0 node::Abort() [node]
 2: 0xa2962e  [node]
 3: 0xae6b1a  [node]
 4: 0xc0251b  [node]
 5: 0xc03ac6  [node]
 6: 0xc04146 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [node]
2
 7: 0x13a5919  [node]
Aborted (core dumped)

The reason, for the abort, as far as I can see, is the fact that the PortDuplex transfers ownership of the buffer chunk over to the main thread using a transferList, which highlights a significant grey areas with the Streams API ... when using a Writable, and passing a Buffer, who takes ownership responsibility for that Buffer? The caller or the receiver? I don't think we have a clear answer for that but we definitely need a fix that does not trigger an Abort.

jasnell · 2020-05-04T21:26:11Z

Refs: nodejs/node#33240

jasnell · 2020-05-04T21:36:48Z

I've updated the example to disable the transfer list in the custom duplex by default to avoid the Abort

addaleax

The code LGTM but I’m not really sure about this example as it exemplifies several things that are not best practices, including using Workers for file I/O and compression, both of which are already asynchronous operations in Node.js.

If you want a more real-world example, maybe a simple text-processing Worker (e.g. applying a complex regexp to a stream – even just filtering out valid JS identifiers is quite heavy if you want to do it correctly) would be better?

addaleax · 2020-05-04T21:38:09Z

examples/stream/port_duplex.js

+  _read () {
+    // Do nothing here. A more complete example would
+    // implement proper read/pause behavior.
+  }


Btw, I’ve thought about porting the code from our internal Worker stdio implementation to npm. This is definitely giving more motivation for that :)

Likely would be good I think. I'm considering the possibility of Piscina providing a more complete version of PortDuplex out of the box.

@jasnell The thing is, in the end streaming use cases for Workers are even rarer than use cases for Workers in general. I don’t think we should encourage this pattern if users don’t really need it.

That's absolutely fair. The key reason I'm thinking through this is to explore the various good and bad ways of using the workers and what the various caveats are.

jasnell · 2020-05-04T22:01:07Z

... it exemplifies several things that are not best practices, including using Workers for file I/O and compression

Yep, this is actually a lead in to some react server side rendering tests I'm looking at but wanted to make sure the basic idea for the PortDuplex would be sound.

jasnell · 2020-05-04T22:04:15Z

(to be clear, I'm not in a rush to land this particular example, we can keep tweaking it until it's demonstrating the right thing)

addaleax · 2020-05-04T22:07:44Z

@jasnell I’ll try to port over the stream code from core, and I’ll try to look into performance improvements there – theoretically, the same Atomics improvements we do for task posting should also make sense for streams, and probably even more so there.

mafintosh · 2020-05-05T08:01:07Z

examples/stream/port_duplex.js

+    if (typeof chunk === 'string') {
+      chunk = Buffer.from(chunk, encoding);
+    }
+    const transferList = this.#transfer ? [chunk.buffer] : undefined;


If the buffer was allocated with allocUnsafe this will cause problems due to pooling no?

Well, there are many ways this is problematic. I'll be updating this such that we always create a new Uint8Array copy of the Buffer then transferring that new copy. That causes the fewest headaches overall.

addaleax · 2020-05-05T13:19:16Z

examples/stream/port_duplex.js

+    // 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.


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 :)

mcollina

I think this example is complicated enough to either live in its own module or be a part of this one.

In case I would recommend the name https://www.npmjs.com/package/scivolo (water slide).

mcollina · 2020-05-05T18:31:20Z

examples/stream/port_duplex.js

+      // TODO(@jasnell): A more complete example would
+      // handle this error more appropriately.
+      this.#port.close();
+      console.error(err);


you should call the callback here.

mcollina · 2020-05-05T18:32:44Z

examples/stream/port_duplex.js

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


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

mcollina · 2020-05-05T18:33:19Z

examples/stream/port_duplex.js

+    //    will cause unexpected and undefined behavior that
+    //    will likely crash the Node.js process.
+    this.#port.postMessage(chunk);
+    callback();


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

Absolutely it should

jasnell · 2020-05-05T19:11:24Z

I think this example is complicated enough to either live in its own module or be a part of this one.

I agree. It should definitely live in it's own module and we should be very prescriptive about how and why to use it. @addaleax, I know you were looking at porting the code we use in core to wrap stdio for workers, let's bundle that into the new project. I'll get that repo started by later tonight.

jasnell · 2020-05-05T20:55:04Z

Converted this PR to a draft so it doesn't get landed. Will be iterating on this further.

jasnell · 2020-05-08T21:11:39Z

This is going to be revisited later.

addaleax reviewed May 4, 2020

View reviewed changes

Add stream example

961a4e2

jasnell force-pushed the stream-example branch from 39a4b22 to 961a4e2 Compare May 4, 2020 23:23

mafintosh reviewed May 5, 2020

View reviewed changes

No transfer

7645f1c

addaleax reviewed May 5, 2020

View reviewed changes

mcollina reviewed May 5, 2020

View reviewed changes

jasnell added 2 commits May 5, 2020 13:41

fixup! No transfer

4c23089

fixup! fixup! No transfer

ffb578b

jasnell marked this pull request as draft May 5, 2020 20:54

jasnell closed this May 8, 2020

metcoder95 deleted the stream-example branch September 17, 2023 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stream example #34

Add stream example #34

jasnell commented May 4, 2020 •

edited

jasnell commented May 4, 2020

jasnell commented May 4, 2020

addaleax left a comment

addaleax May 4, 2020

jasnell May 4, 2020

addaleax May 4, 2020

jasnell May 4, 2020

jasnell commented May 4, 2020

jasnell commented May 4, 2020

addaleax commented May 4, 2020

mafintosh May 5, 2020

jasnell May 5, 2020

addaleax May 5, 2020

mcollina left a comment

mcollina May 5, 2020

mcollina May 5, 2020

mcollina May 5, 2020

jasnell May 5, 2020

jasnell commented May 5, 2020

jasnell commented May 5, 2020

jasnell commented May 8, 2020

Add stream example #34

Add stream example #34

Conversation

jasnell commented May 4, 2020 • edited

jasnell commented May 4, 2020

jasnell commented May 4, 2020

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented May 4, 2020

jasnell commented May 4, 2020

addaleax commented May 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented May 5, 2020

jasnell commented May 5, 2020

jasnell commented May 8, 2020

jasnell commented May 4, 2020 •

edited