Skip to content

reject parsing promise on stream error #2427

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

Closed
wants to merge 1 commit into from
Closed

reject parsing promise on stream error #2427

wants to merge 1 commit into from

Conversation

matthieusieben
Copy link
Contributor

Sorry of this does not make sense but reading the code, it feels wired to allow forever pending promises.

@matthieusieben
Copy link
Contributor Author

Actually IMHO, it is even wiered that this was not implemented as a stream transform or an async iterable?

@brianc
Copy link
Owner

brianc commented Dec 3, 2020

Actually IMHO, it is even wiered that this was not implemented as a stream transform or an async iterable?

yeah there are things that are more difficult to do w/ the stream api, and I didn't want to be confined to using things in a 'streaming' way for parsing purposes as I want to be able to tweak whatever I need to to make things as fast as possible, and in my experience that means "getting things off the stream as fast as I can and then do whatever I need to with them." It's also been easier to maintain backwards compatibility with many multi-year long versions of node, including v8.x line of node which isn't even LTS supported officially by the node team.

it feels wired to allow forever pending promises.

Yeah, I should have put a comment there - the only reason that promise exists is to aid in testing. Stream error handling is attached outside. Are you consuming this library/class directly? If you are I'd love to hear more of your painpoints & see if there are things that can make the API better (like docs!) - it has so far been a way to abstract out client/server protocol, but its a bit fiddly in how you use it because performance is # 1 priority above any API elegance.

The tests are failing in some versions of CI btw. This PR also is going to cause problems because of this. The library doesn't even use this promise (it's only used in testing to allow for tests to wait for a stream to be fully consumed). but if you add a rejection to it this will cause unhandledRejectionError messages any time the stream errors because the library itself isn't even handling the promise. If you want, a better approach might be to drop the use of the promise all together & change the tests to be able to wait for the end of the stream (since the tests supply a test stream).

@matthieusieben
Copy link
Contributor Author

Nevermind my PR.

I just tought it was really wiered to have a potentially never resolving promise in the code. This might cause memory leaks though I didn't encounter one because of this (I think).

@matthieusieben matthieusieben deleted the patch-2 branch December 6, 2020 20:40
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