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

Fix pg-query-stream implementation #2051

Merged
merged 5 commits into from
Dec 30, 2019
Merged

Fix pg-query-stream implementation #2051

merged 5 commits into from
Dec 30, 2019

Conversation

brianc
Copy link
Owner

@brianc brianc commented Dec 28, 2019

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! 🙏

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, and `highWaterMark` is no longer supported.  It shouldn't impact most usage, but breaking regardless.
@brianc
Copy link
Owner Author

brianc commented Dec 28, 2019

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
Copy link
Contributor

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
Copy link
Owner Author

brianc commented Dec 28, 2019 via email

@matthieusieben
Copy link
Contributor

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

@matthieusieben
Copy link
Contributor

matthieusieben commented Dec 28, 2019

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
Copy link
Owner Author

brianc commented Dec 28, 2019

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

@brianc
Copy link
Owner Author

brianc commented Dec 29, 2019

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
Copy link
Contributor

matthieusieben commented Dec 29, 2019

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
Copy link
Contributor

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
Copy link
Owner Author

brianc commented Dec 29, 2019

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
Copy link
Contributor

matthieusieben commented Dec 29, 2019

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
Copy link
Owner Author

brianc commented Dec 29, 2019

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
Copy link
Contributor

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
Copy link
Contributor

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

@brianc
Copy link
Owner Author

brianc commented Dec 29, 2019

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
Copy link
Owner Author

brianc commented Dec 29, 2019

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
Copy link
Owner Author

brianc commented Dec 29, 2019

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
Copy link
Owner Author

brianc commented Dec 29, 2019

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
Copy link
Contributor

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
Copy link
Contributor

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

@brianc
Copy link
Owner Author

brianc commented Dec 29, 2019

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
Copy link
Owner Author

brianc commented Dec 30, 2019

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! 🙏

@brianc brianc merged commit 8eca181 into master Dec 30, 2019
@brianc brianc deleted the bmc/fix-pg-query-stream branch December 30, 2019 05:08
@matthieusieben
Copy link
Contributor

You're welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants