Implement Readable instead of Stream #740

zbjornson · 2016-03-19T22:40:01Z

Would benefit from some testers (@mitar?) and feedback...

Both JPEGStream and PNGStream currently inherit from stream.Stream, not stream.Readable. As a consequence, they are missing a bunch of methods. (See #674, #232.) They will also start flowing without a listener, but because the stream is wrapped in process.nextTick I think that would only be noticed from the node prompt.

It was straightforward to switch to Readable. I didn't attempt to make the C++ code wait to queue more data after the stream starts (first call to .read); after the first chunk it pushes data into the stream queue without waiting for the stream consumer to call .read (directly or indirectly). There should be no lost bytes as a result of flowing without a sink anymore, but consumers could see more memory consumption from the queuing of bytes that previously would have been lost into the ether.

I'm still confused about the "async" vs "sync" names of these APIs -- they all emit chunks of data within callbacks, which seems async to the consumer, if not internally.

LinusU · 2016-03-21T08:39:22Z

test/canvas.test.js

@@ -749,6 +749,7 @@ describe('Canvas', function () {
 it('Canvas#createSyncPNGStream()', function (done) {
 var canvas = new Canvas(20, 20);
 var stream = canvas.createSyncPNGStream();
+ assert(stream instanceof require("stream").Readable);


Single quotes please :)

LinusU · 2016-03-21T08:41:47Z

Awesome 🙌 I've wanted to do this myself for quite some time, I think that this looks very good.

I think the point with "async" vs "sync" is that "sync" is being run on the main thread, although calling back to javascript land in chunks? Haven't looked at the code properly to say though.

In my opinion we shouldn't have sync streams at all since that seems like a very weird concept.

zbjornson · 2016-03-21T20:23:02Z

Thanks!

I forgot that node 0.8 doesn't have stream.Readable to extend. @rvagg has a nice article about compatibility here: https://r.va.gg/2014/06/why-i-dont-use-nodes-core-stream-module.html. Drop 0.8 or make it compatible?
Apparently 0.10 will miss the 'end' event if it is attached after the 'data' event is attached -- or at least, when I re-ordered the event listener attachments in the toDataURL method and tests, it worked. This could be breaking for consumers. I don't see anything about this in the 0.10 docs, and some quick googling didn't reveal any relevant bugs that were fixed in 0.12. https://nodejs.org/docs/v0.10.43/api/stream.html#stream_compatibility_with_older_node_versions.

LinusU · 2016-03-25T12:16:51Z

test/canvas.test.js

@@ -749,26 +749,35 @@ describe('Canvas', function () {
 it('Canvas#createSyncPNGStream()', function (done) {
 var canvas = new Canvas(20, 20);
 var stream = canvas.createSyncPNGStream();
+ assert(stream instanceof require('stream').Readable);


Could you move the require call to the top of the file? You could still do var Readable = require('stream').Readable

LinusU · 2016-03-25T12:42:57Z

Hmm

I don't have a problem with dropping 0.8 support, according to this tweet and conversation, almost no one is using 0.8 anymore. https://twitter.com/seldo/status/712386414902521861

It should be a major bump though.

The problems on 0.10 is a bit disturbing. Would it be possible for us to delay the first data event until the next tick?

calvinmetcalf · 2016-03-26T13:02:44Z

Maybe the data event is causing the stream to synchronously drain before you get around to putting the end listener.

mcollina · 2016-03-29T13:57:02Z

I agree with @calvinmetcalf. In streams 2, if all the data is available in JS-land and a 'data' handler is added, it would be completely synch, and you will add the 'end' event after 'end' was already happened. Does createSyncPNGStream create all the data synchronously?

zbjornson · 2016-03-29T16:53:46Z

@calvinmetcalf and @mcollina that appears to be what's happening -- thanks.

I was hoping to find documentation about this change between 0.10 and 0.12 just to confirm that I wasn't doing something wrong. I think this is the diff that caused the behavior change:
nodejs/node-v0.x-archive@0f8de5e#diff-ba6a0df0f5212f5cba5ca5179e209a17L681
Notice .resume calls through a nextTick, whereas the removed emitDataEvents looks like it plowed straight through.

Anyway, wrapping it in a nextTick now...

zbjornson · 2016-03-29T16:56:01Z

From nodejs/node-v0.x-archive#5865 (comment)

The other semantic change is that adding a readable event handler will not cause readable to be emitted until nextTick

Indeed.

zbjornson · 2016-03-29T17:14:56Z

Passing in 0.10 and later. Undid the changes to toDataUrl and tests.

Node 5 doesn't seem to need to be an allowed failure anymore, btw.

LinusU · 2016-03-30T19:34:58Z

lib/jpegstream.js

+ process.nextTick(function(){
+ canvas[method](options.bufsize, options.quality, options.progressive, function(err, chunk){
+ if (err) {
+ self.emit('error', err);


wrong indentation

mcollina · 2016-05-10T08:43:30Z

lib/jpegstream.js

+ });
+ });
+ }
+ };


allocating read here or in the prototype has few differences, apart from style. I prefer to use prototype when dealing with a class an inheriting.

Regarding this block, I would consider removing the process.nextTick call. Plus, I think it's possible to recycle (declare inside the constructor) the function passed to canvas[method].

The golden rule is "try not to allocate functions inside _read, _write or _transform". That is usually a very hot path and all those function allocation will slow you down.

From earlier, we must use process.nextTick.

Sorry for not being more clear, or for still not understanding what you're saying, but this is not a hot path because it has the surrounding if block that causes evaluation of the function expression exactly once, thus there is nothing to reuse. I think then the only difference is style, but please let me know if I'm missing something.

As far as size of npm/inherits vs util.inherits, this is a node module only.

mcollina · 2016-05-10T08:44:38Z

If you care about browser size, I suggest you using inherits from NPM instead of util.inherits.

zbjornson · 2016-05-10T18:37:31Z

Updated travis.yaml: remove node 0.8, add 6.x and no longer allow 5.x to fail. CI green!

zbjornson · 2016-09-24T18:06:08Z

(Needs update since PDF stream was added)

LinusU · 2016-10-23T12:05:13Z

@zbjornson sorry for the delay on this, I've now cut a 1.x branch from master and from now on master is for Canvas 2.x. I'm dropping 0.8 support in #825 so this should be good to merge 👍

zbjornson · 2016-10-24T03:24:41Z

No problemo, will fix conflicts and add PDF stream this week. :)

zbjornson · 2016-10-31T00:01:37Z

@LinusU updated :)

chearon · 2016-10-31T13:04:22Z

I do think the comments by @mcollina deserve another look, can _read be defined on the prototype? If we drop 0.10, does that mean the nextTick stuff can be removed? It seems like it is not an issue in streams3

zbjornson · 2016-10-31T19:36:10Z

If 0.10 is dropped, nextTick can be removed, yes.

I had _read in the ctor so that hasStarted is not visible, but that's a small reason. There's no difference in stream performance, only in stream construction:

As it was: construct 1,430,136 ops/sec, end event 576 ops/sec
As it was with guard clause reversed (if (hasStarted) return): 1,415,082 & 573
On proto: 8,442,640 & 574
On proto, reassign _read = noop on first _read call: 8,452,849 & 580
nextTick removed: no change

The 4th option (reassign to noop) is the best of both worlds -- no internal properties exposed and fast construction. Updated PR to use that method.

zbjornson · 2016-11-22T02:28:00Z

@LinusU gentle nudge, from my side this is ready for merge into 2.0. :)

LinusU · 2016-11-22T09:36:03Z

Sorry for the delay, I hope to get some time this week 👍

LinusU · 2017-05-03T21:54:19Z

Sorry for the delay, awesome work!

zbjornson · 2017-05-03T22:00:25Z

TODO at myself (or anyone else who wants to do it): since 0.10 is dropped now, the nextTick stuff can be removed, per discussion above.

See discussion in Automattic#740

See Automattic#740 (comment)

zbjornson mentioned this pull request Mar 19, 2016

[TypeError: Object #<PNGStream> has no method 'destroy'] #232

Closed

LinusU reviewed Mar 21, 2016
View reviewed changes

zbjornson force-pushed the stream-polishing branch 3 times, most recently from dbee175 to 871f010 Compare March 21, 2016 20:07

LinusU reviewed Mar 25, 2016
View reviewed changes

LinusU mentioned this pull request Mar 25, 2016

Canvas 2.0 🚀 #743

Closed

zbjornson mentioned this pull request Mar 25, 2016

Readable stream behavior in 0.10 nodejs/help#120

Closed

zbjornson force-pushed the stream-polishing branch from 871f010 to 6e603f1 Compare March 29, 2016 17:06

LinusU reviewed Mar 30, 2016
View reviewed changes

zbjornson force-pushed the stream-polishing branch 2 times, most recently from 6a0ebb7 to 76787e1 Compare May 9, 2016 19:43

zbjornson mentioned this pull request May 9, 2016

Better streams that support being returned asynchronously. #765

Closed

mcollina reviewed May 10, 2016
View reviewed changes

zbjornson force-pushed the stream-polishing branch 2 times, most recently from ac615b7 to 4cacbe8 Compare May 10, 2016 18:24

zbjornson force-pushed the stream-polishing branch from 4cacbe8 to dc706ec Compare May 10, 2016 19:11

zbjornson mentioned this pull request Jun 10, 2016

Crude PDF stream implementation #781

Merged

LinusU mentioned this pull request Oct 23, 2016

Drop support for Node.js 0.8 #825

Merged

LinusU mentioned this pull request Oct 30, 2016

Is it time to drop Node.js 0.10 support (in 2.x) #832

Closed

zbjornson force-pushed the stream-polishing branch 2 times, most recently from f288b05 to fb84653 Compare October 31, 2016 00:00

Implement Readable instead of Stream.

8b5dc05

zbjornson force-pushed the stream-polishing branch from fb84653 to 8b5dc05 Compare October 31, 2016 19:34

LinusU merged commit 382fb07 into Automattic:master May 3, 2017

LinusU mentioned this pull request May 3, 2017

suppress duplicate end events #679

Closed

zbjornson deleted the stream-polishing branch May 3, 2017 21:59

zbjornson added a commit to zbjornson/node-canvas that referenced this pull request May 4, 2017

Remove nextTicks since they were only needed by node 0.10

1ecc702

See discussion in Automattic#740

zbjornson mentioned this pull request May 4, 2017

Remove source support for node < 4 #918

Merged

zbjornson added a commit to zbjornson/node-canvas that referenced this pull request Jun 20, 2017

Remove nextTicks since they were only needed by node 0.10

2bd2f7c

See discussion in Automattic#740

zbjornson added a commit to zbjornson/node-canvas that referenced this pull request Jun 20, 2017

fix merge error in pngstream.js

e5c8308

See Automattic#740 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Readable instead of Stream #740

Implement Readable instead of Stream #740

zbjornson commented Mar 19, 2016

LinusU Mar 21, 2016

LinusU commented Mar 21, 2016

zbjornson commented Mar 21, 2016

LinusU Mar 25, 2016

LinusU commented Mar 25, 2016

calvinmetcalf commented Mar 26, 2016

mcollina commented Mar 29, 2016

zbjornson commented Mar 29, 2016

zbjornson commented Mar 29, 2016

zbjornson commented Mar 29, 2016

LinusU Mar 30, 2016

mcollina May 10, 2016

zbjornson May 10, 2016

mcollina commented May 10, 2016

zbjornson commented May 10, 2016

zbjornson commented Sep 24, 2016

LinusU commented Oct 23, 2016

zbjornson commented Oct 24, 2016

zbjornson commented Oct 31, 2016

chearon commented Oct 31, 2016

zbjornson commented Oct 31, 2016

zbjornson commented Nov 22, 2016

LinusU commented Nov 22, 2016

LinusU commented May 3, 2017

zbjornson commented May 3, 2017

Implement Readable instead of Stream #740

Implement Readable instead of Stream #740

Conversation

zbjornson commented Mar 19, 2016

LinusU Mar 21, 2016

Choose a reason for hiding this comment

LinusU commented Mar 21, 2016

zbjornson commented Mar 21, 2016

LinusU Mar 25, 2016

Choose a reason for hiding this comment

LinusU commented Mar 25, 2016

calvinmetcalf commented Mar 26, 2016

mcollina commented Mar 29, 2016

zbjornson commented Mar 29, 2016

zbjornson commented Mar 29, 2016

zbjornson commented Mar 29, 2016

LinusU Mar 30, 2016

Choose a reason for hiding this comment

mcollina May 10, 2016

Choose a reason for hiding this comment

zbjornson May 10, 2016

Choose a reason for hiding this comment

mcollina commented May 10, 2016

zbjornson commented May 10, 2016

zbjornson commented Sep 24, 2016

LinusU commented Oct 23, 2016

zbjornson commented Oct 24, 2016

zbjornson commented Oct 31, 2016

chearon commented Oct 31, 2016

zbjornson commented Oct 31, 2016

zbjornson commented Nov 22, 2016

LinusU commented Nov 22, 2016

LinusU commented May 3, 2017

zbjornson commented May 3, 2017