Fix pg-query-stream implementation

[brianc]

There were some subtle behaviors with the stream being implemented incorrectly & not working as expected with async iteration. I've modified the code based on #2050 and comments in #2035 to have better test coverage of async iterables and update the internals significantly to more closely match the readable stream interface.

Note: this is a breaking (semver major) change to this package as the close event behavior is changed slightly so it now emits always, instead of improperly being suppressed when manually closing the stream, and highWaterMark is no longer supported. It shouldn't impact most usage, but breaking regardless.

cc @matthieusieben - I think I covered the tests you were looking for in your previous comment, but please let me know if I missed anything! 🙏

[brianc]

Hmmm....a test is failing on node carbon which falls out of LTS in 2 days and will be dropped from pg's support matrix soon after. I wonder if it's just a stream implementation difference on older versions of node? I'll look into this over the weekend & get it figured out...likely just skip that test in the case of node<10.

[matthieusieben]

The code from #2050 is slightly different than the one I published before. I removed the support for multiple callbacks, as it should not be needed.

[brianc]

k cool thanks a ton! ill get them synced up

…

On Sat, Dec 28, 2019 at 11:34 AM Matthieu Sieben ***@***.***> wrote: The code from #2050 <#2050> is slightly different than the one I published before. I removed the support for multiple callbacks, as it should not be needed. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2051?email_source=notifications&email_token=AAAMHIMSDIYS3KC64JJQWIDQ26EYRA5CNFSM4KAPTIIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHYOTHY#issuecomment-569436575>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAMHIN6MJUI3PTIIPFN7GDQ26EYRANCNFSM4KAPTIIA> .

[matthieusieben]

Another thing: since you are updating the semver, maybe you could remove the close() function all together? Streams should be destroy()'d!

[matthieusieben]

Another comment, I am not sure what to do with the err argument in the destroy. My supposition is that the error can be used to determine how to properly destroy depending on the error type (eg. Destroyed from within, during a read, or from outside). In either case it is not needed to keep an internal reference to err for the stream implementation.

[brianc]

great points - i'll do a bit more refactoring and 🔨 on this tomorrow!

[brianc]

Okay came back & updated this more. Removed the close method entirely in favor of destroy, reduced the complexity of error handling. Removed the code that tried to prevent calling destroy multiple times as node does this for you now it seems. Update tests to cover more edge cases of async iterator and error & close conditions. This is absolutely a semver major bump, but should reduce edge cases significantly as this module is now doing very little outside of very tersely attaching pg-cursor to a readable stream.

[matthieusieben]

Mmmh you removed too much. The _reading was preventing to close the cursor while reading. Not sure how but I did get a connection in an invalid state because of this.

[matthieusieben]

Also, not sure that you need to call the destroy callback with the provided error. If the stream is destroyed because of an outside error, closing the cursor does not necessarily results in an error. It seems odd to tell the Readable class that "there was an error when destroying the stream" when the error actually originated elsewhere

[brianc]

Also, not sure that you need to call the destroy callback with the provided error. If the stream is destroyed because of an outside error, closing the cursor does not necessarily results in an error. It seems odd to tell the Readable class that "there was an error when destroying the stream" when the error actually originated elsewhere

Sorry I'm not following what you're saying here exactly.

[matthieusieben]

Also, if you read the docs: https://nodejs.org/api/stream.html#stream_writable_destroy_err_callback
The _destroy callback argument is not an optional argument (no need to check if exists)

You should re-chek the code from my PR with this in mind

[brianc]

Mmmh you removed too much. The _reading was preventing to close the cursor while reading. Not sure how but I did get a connection in an invalid state because of this.

Yeah I tried to get this to trip w/ various tests but wasn't able to do so...I can put the code back in but feels weird to have code which is effectively not tested against properly since adding or removing doesn't effect the test results.

[matthieusieben]

Well what does the pg doc says about closing a cursor while reading? And what would happen in the pg-cursor class if more data comes in after it was closed

[matthieusieben]

cb(err || _err) => why call cb with the error _err?

[brianc]

The docs are a bit unclear on what to pass as an error argument to the callback passed to _destroy - do you pass the error passed to _destroy? or do you pass an error which may have originated during your destroy operation? In this case I'm passing through the error passed in to _destroy back to the callback or passing an error which originated by calling close on the cursor. Probably best way to fix this is to write tests for the expected behavior as IMO the docs aren't entirely clear but I'd rather expose all the errors possible in that case rather than not propagate them back.

[brianc]

cb(err || _err) => why call cb with the error _err?

I added a test for this behavior which fails if I remove calling cb with _err and I think it is in compliance with what should happen where calling .destroy(new Error('foo')) will cause the stream to destroy itself and emit an error event w/ the supplied error.

[brianc]

Well what does the pg doc says about closing a cursor while reading? And what would happen in the pg-cursor class if more data comes in after it was closed

Yeah you're right there are some weird edge cases here...I gotta run some errands but I'll try to get back and write tests for more of these edge cases. It's kinda undefined behavior and/or misuse of the cursor itself if you do something like

const cursor = new PgCursor('SELECT *')
cursor.read(25, () => {})
cursor.close((err) => {})

I quickly tested that and it actually causes some problems so I'll make sure that type of scenario can't be performed through using the stream in any way.

[brianc]

Well what does the pg doc says about closing a cursor while reading? And what would happen in the pg-cursor class if more data comes in after it was closed

Okay did some tests - calling cursor.close while a cursor is reading is fine....it will close the cursor after the next set of results are returned. Since we're using a readable stream the stream implementation itself wont enqueue any of the rows returned after calling destroy. However, if you call .destroy (which calls cursor.close()) before you submit the cursor, it will throw an uncatchable error right now. I added a failing test for this...which I can fix later today. I think in this scenario if you destroy a query stream before you even submit it it should just destroy normally and if you try to submit it in the future it should throw an error saying it's already been destroyed or something like that. What do you think?

[matthieusieben]

That seems about right.

Note that you can rely on the 'close' event, for which you can add a listener in the submit method.

The autoDestroy and emitClose options should make everything work.

[matthieusieben]

Now I wouldn't change the behavior of destroy (throw an exception) as this would be done in the Readable class itself

[brianc]

Now that I finally got eveything in one repo (yay!) it's much easier to fix things in dependent modules and have those fixes be carried into new ones like this!

[brianc]

Okay - I think we're good here. Plan on releasing a semver patch bump of pg-cursor & semver major bump (with changelog entry) for pg-query-stream tomorrow. @matthieusieben thanks so much for your help and patience! 🙏

[matthieusieben]

You're welcome.