Streams3 adaptor(s) #35

Fishrock123 · 2019-06-13T00:27:29Z

It works!!

ReadableSink to turn BOB flow to streams3, WritableSource to turn streams3 flow into BOB.

Implements #18

Test cases: run npm test!
~~(manually verify the output file, readme_ or readme.gz (there is an ungzip helper in tests/).~~
(may require Node 13)

Use NODE_DEBUG=bob to see what is happening.

~~Replace readme.md with your choice of file.~~

R=@mcollina

mcollina

Good work!!!! I've left some notes.

mcollina · 2019-06-14T16:19:05Z

helpers/bob-duplex.js

+    this.cork()
+  }
+
+  _write(chunk, encoding, callback) {


I think we should add support for _writev as well.

I think that should probably wait for if bob supports multi buffer next()s or something. Otherwise it seems more reasonable to just have WritableStream manage it?

+1. We should add multi buffed support.

Should be discussed at #30

helpers/bob-duplex.js

Raynos · 2019-06-19T21:57:17Z

helpers/bob-duplex.js

+      if (this.source !== null) {
+        this.source.pull(err)
+        this[kDestroyCallback] = cb
+      }


Need an else here. If there is no source BUT sink exists then we are responsible for calling this.sink.next(error, status.error, ...)

Actually you have to destroy both the sink and the source always and wait for them both to be finished destroying ...

I think it should just explode if a source doesn't exist, tbh.

helpers/bob-duplex.js

Raynos · 2019-06-19T22:15:38Z

tests/bob-streams3-from-source.js

+const fileSource = new FileSource(process.argv[2])
+const bobDuplex = new BobDuplex({ highWaterMark: 1024, name: '1' })
+
+new Stream(fileSource, bobDuplex)


This BobDuplex API is confusing to me. I think a more common API would be new BobDuplex({ hwm, name, sink, source }).

Calling bindSource() should error because it should be in the constructor instead. Calling bindSink() is fine because that's part of the implementors API and it's necessary to be a valid bob.Source

Yeah agree it's not ideal

helpers/bob-duplex.js

Raynos · 2019-06-19T22:42:38Z

helpers/bob-duplex.js

+    this.sink.next(status_type.continue, null, chunk, chunk.length)
+  }
+
+  _read (size) {


I'm worried _read() will not get called again if we do not call push() with enough data ...

@Raynos I don't really understand... Could you elaborate?

The documentation of Readable
Seems to imply that if you do not push at least size bytes it will not call _read again.

The current implementation seems to be doing one pull call and it might return <size bytes

@mcollina could you possibly confirm this?

The docs seem to say:

The size argument is advisory.

Raynos · 2019-06-19T22:44:35Z

helpers/bob-duplex.js

+
+    // Regular data case.
+    if (status === status_type.continue) {
+      this.push(buffer.slice(0, bytes))


the return type of push() is indicating whether we need to call pull() again and it's currently ignored. I'm worried this stream will get stuck in a weird state where Readable will not call _read again because you didn't push enough data.

The behavior of Readable is different in the use case where pipe() is called vs read() + readable.on('readable', ...) etc

Raynos · 2019-06-19T22:45:30Z

helpers/bob-duplex.js

+    this.sink = sink
+  }
+
+  next (status, error, buffer, bytes) {


I'm pretty sure that if bytes < buffer.length we are supposed to call pull() again for this to be a correctly implemented Readable with _read

Raynos · 2019-06-19T22:46:24Z

helpers/bob-duplex.js

+    }
+
+    // If we have an error and have not already been in error state, emit it Streams3 style.
+    if (error !== null) {


checking status === status_type.error is nicer then check if error !== null

Raynos · 2019-06-19T23:04:16Z

I started reading this, readable-stream, fs-sink, fs-source & pull-stream-to-stream

I read some comments in readable-stream that I knew were there 5 years ago and I read them in isaacs voice, there are many mode and many tests to write.

Please use https://github.com/dominictarr/stream-spec and friends. These data back to 2015 and was in the streams npm era where readable-stream did not exist and we were out of luck, we had to implement Readable, Writable & Duplex by hand. We had to implement the really old streams1 api of 'data', 'end', write, end, pipe, etc.

There is no stream-spec module for streams2 or streams3 that I can find.

Raynos · 2019-06-19T23:08:37Z

Talking of spec, can we port these to bob

I believe Verify is a partial implementation of it.

Fishrock123 · 2019-07-08T14:00:08Z

This BobDuplex API is confusing to me. I think a more common API would be new BobDuplex({ hwm, name, sink, source }).

(#35 (comment))

@Raynos What if this extended / was the (bob) Stream() utility?

It looks like e,g, const stream = new Stream(streamSource, new PassThrough(), streamSink) and maybe it or some version could be a streams3-able? (Note: Idk if that would actually work but it's worth thinking about).

Fishrock123 · 2019-07-11T22:51:45Z

Updated with added automated tests!

Fishrock123 · 2019-07-16T20:13:34Z

Ok so since there's some comments here about the API design in general, here's The Dream:

get rid of BobDuplex
build core streams on top of bob
make 'the old' core Stream() api take source, ...sinks arguments

Basically this isn't ideal but it is an important between step.

Refs: #34 PR-URL: #35

Raynos · 2019-07-17T17:38:47Z

We will need BobDuplex for backcompat.

I imagine for node core we want to expose new APIs that use bob streams and deprecate streams3 APIs.

I don't think we can make a breaking change, we will always need streams3 and thus BobDuplex.

Also for things like http it might just be too little, too late and we can only implement bob streams APIs for http2

Fishrock123 · 2019-07-17T17:44:00Z

I think you're maybe missing the idea.

The idea is and always has been to implement Readable and Writable on top of "bob streams".

Which is probably the only way we can actually get a reasonable transition to happen.

Raynos · 2019-07-17T17:45:03Z

The idea is and always has been to implement Readable and Writable on top of "bob streams".

BobDuplex implements the Readable and Writable API; i guess you can implement a BobReadable and BobWritable if we wanted to.

Fishrock123 · 2019-07-17T17:47:32Z

Correct, but it is technically then the wrong way around - it takes a Duplex and implements Bob connectors on top.

Using that then, you can stick it "inbetween" a bob stream and a streams3 stream to connect them.

Ideally though, that wouldn't be necessary - you'd just set your stream up and it both would be a streams3 stream and also streams3 streams would have the bob api "underneath".

Refs: #34 PR-URL: #35

Refs: #35 (comment)

Tests pass, seems unnecessary as per comments. Just in case, though, I want to keep these in-place.

oof

yeah, there's no standard way to do this

this is much better! \o/

for version pinning stability

Fishrock123 · 2019-07-22T23:33:51Z

@Raynos @mcollina Updated, and split from BobDuplex into ReadableSink and WritableSource.

Hopefully the api is more clear and less confusing, and also more separated and reasonable to understand. I still need to do some state cleanup, I think.

Should handle things a bit better.

This error handling seems more correct to me.

Fishrock123 · 2019-07-23T19:25:07Z

Ok, clean up pushed. Error handling and some other state stuff should be more correct now, I think.

Fishrock123 · 2019-07-23T19:26:15Z

helpers/writable-source.js

+    const writeCb = this[kWriteCallback]
+    this[kWriteCallback] = null
+
+    // If we don't yet have a write callback from an attempted Streams3 write, just bail.


One thing I don't know what to do about though, is this.

I can't get it to work without bailing in this case no matter how the streams are composed in user code...

See nodejs/node#29653

Raynos

Readable class looks legit, a bunch of comments for the Writable class.

Raynos · 2019-10-17T01:27:18Z

helpers/writable-source.js

+    this[kPulling] = false
+
+    // Send data to our sink.
+    this.sink.next(status_type.continue, null, chunk, chunk.length)


Do we want to implement buffering here ? Same in the read case. We may already be "piping" data before we have hooked up the sinks and sources.

If not; add a check for this.sink and throw some kind of readable error about writing to stream before new Stream() or source.bindSink() was called.

Raynos · 2019-10-17T01:30:57Z

helpers/writable-source.js

+      const encoding = this._readableState.encoding || this._readableState.defaultEncoding
+      chunk = Buffer.from(chunk, encoding)
+    }
+


I suspect this needs an if check here if (this[kPulling]) { cb() } else { this[kWriteCallbac] = callback }

Raynos · 2019-10-17T01:31:33Z

helpers/writable-source.js

+    this[kPulling] = false
+
+    // Send data to our sink.
+    this.sink.next(status_type.continue, null, chunk, chunk.length)


If this[kPulling] is false here then we do not want to call this.sink.next() yet; we want to store the current chunk in this[kPendingChunk]

These two classes are a way of interop between BOB and Streams3. Closes: #18 PR-URL: #35

Fishrock123 · 2019-12-11T18:51:34Z

Landed ReadableSink and WritableSource in 57a78d1

mcollina reviewed Jun 14, 2019

View reviewed changes

Raynos requested changes Jun 19, 2019

View reviewed changes

Fishrock123 force-pushed the streams3-adaptor branch from 7f27eca to f48c3f6 Compare July 11, 2019 22:50

Fishrock123 changed the base branch from master to automated-tests July 11, 2019 22:50

Fishrock123 force-pushed the automated-tests branch from 551ea5e to 8ec6b97 Compare July 15, 2019 16:57

Fishrock123 changed the base branch from automated-tests to master July 15, 2019 17:26

Fishrock123 force-pushed the streams3-adaptor branch from f48c3f6 to aa251c1 Compare July 15, 2019 17:27

Fishrock123 force-pushed the master branch from 587a8b8 to f7e3d0f Compare July 15, 2019 18:38

Fishrock123 changed the base branch from master to verification-passthrough July 16, 2019 20:27

Fishrock123 force-pushed the streams3-adaptor branch from aa251c1 to 0fe264f Compare July 16, 2019 20:27

Fishrock123 added a commit that referenced this pull request Jul 16, 2019

tests: use Verify() with streams3 tests

0fe264f

Refs: #34 PR-URL: #35

Fishrock123 force-pushed the verification-passthrough branch from 9d2a491 to c5a7502 Compare July 17, 2019 17:34

Fishrock123 changed the base branch from verification-passthrough to master July 17, 2019 17:47

Fishrock123 force-pushed the streams3-adaptor branch from 0fe264f to 50f2c80 Compare July 17, 2019 17:49

Fishrock123 added a commit that referenced this pull request Jul 17, 2019

tests: use Verify() with streams3 tests

50f2c80

Refs: #34 PR-URL: #35

Fishrock123 force-pushed the streams3-adaptor branch from 50f2c80 to 7c86655 Compare July 18, 2019 17:09

Fishrock123 added a commit that referenced this pull request Jul 18, 2019

tests: use Verify() with streams3 tests

7c86655

Refs: #34 PR-URL: #35

Fishrock123 added 3 commits July 22, 2019 11:34

helpers: wip bob-duplex (not fully working)

d7c4c5c

BobDuplex automated tests

2416775

tests: use Verify() with streams3 tests

985f2f5

Refs: #34 PR-URL: #35

Fishrock123 force-pushed the streams3-adaptor branch from 7cbf4e1 to 985f2f5 Compare July 22, 2019 18:34

Fishrock123 mentioned this pull request Jul 22, 2019

Non-single-logical-flow (multiple pulls) #30

Open

Fishrock123 added 6 commits July 22, 2019 13:00

fix _destroy possible FD leak

a20ba30

Refs: #35 (comment)

comment out corking

764231d

Tests pass, seems unnecessary as per comments. Just in case, though, I want to keep these in-place.

delay final callback and sink ending

1856b9d

oof

deal with encoding 'correctly'

381949c

yeah, there's no standard way to do this

split BobDuplex into ReadableSink and WritableSource

76ef34d

this is much better! \o/

use readable-stream instead of core

70e7117

for version pinning stability

Fishrock123 added 2 commits July 23, 2019 12:23

cleanup / improve ReadableSink

3a98b02

Should handle things a bit better.

cleanup / improve WritableSource

7e2ec35

This error handling seems more correct to me.

Fishrock123 requested a review from jasnell July 23, 2019 19:24

Fishrock123 commented Jul 23, 2019

View reviewed changes

This was referenced Jul 23, 2019

Progress 23/07/2019 - July #40

Closed

Is it known if bob (Streams+) is expected to be compatible with pull-stream, or vice versa? pull-stream/pull-stream#116

Open

Fishrock123 mentioned this pull request Sep 22, 2019

Dealing with async errors during _read() nodejs/node#29584

Closed

Fishrock123 requested a review from Raynos September 23, 2019 17:46

fixup: always use destroy(err) instead of emit

bd6a3d1

See nodejs/node#29653

Raynos approved these changes Oct 17, 2019

View reviewed changes

Fishrock123 changed the title ~~Streams3 adaptor~~ Streams3 adaptor(s) Dec 11, 2019

Fishrock123 added a commit that referenced this pull request Dec 11, 2019

helpers: add WritableSource and ReadableSink

57a78d1

These two classes are a way of interop between BOB and Streams3. Closes: #18 PR-URL: #35

Fishrock123 closed this Dec 11, 2019

Fishrock123 mentioned this pull request Dec 11, 2019

Potential streams3 adaptor bugs #60

Open

Streams3 adaptor(s) #35

Streams3 adaptor(s) #35

Conversation

Fishrock123 commented Jun 13, 2019 • edited Loading

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

mcollina Jun 14, 2019 • edited by Fishrock123 Loading

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

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

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

Raynos commented Jun 19, 2019

Raynos commented Jun 19, 2019

Fishrock123 commented Jul 8, 2019

Fishrock123 commented Jul 11, 2019

Fishrock123 commented Jul 16, 2019

Raynos commented Jul 17, 2019

Fishrock123 commented Jul 17, 2019 • edited Loading

Raynos commented Jul 17, 2019

Fishrock123 commented Jul 17, 2019

Fishrock123 commented Jul 22, 2019 • edited Loading

Fishrock123 commented Jul 23, 2019

Choose a reason for hiding this comment

Raynos 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

Fishrock123 commented Dec 11, 2019

Fishrock123 commented Jun 13, 2019 •

edited

Loading

mcollina Jun 14, 2019 •

edited by Fishrock123

Loading

Fishrock123 commented Jul 17, 2019 •

edited

Loading

Fishrock123 commented Jul 22, 2019 •

edited

Loading