Skip to content

fix: Prevent closing the portal twice #2609

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

Merged
merged 1 commit into from
Jan 28, 2022
Merged

fix: Prevent closing the portal twice #2609

merged 1 commit into from
Jan 28, 2022

Conversation

matthieusieben
Copy link
Contributor

@matthieusieben matthieusieben commented Sep 7, 2021

Followup of #2120
Fixes #2119

@brianc
Copy link
Owner

brianc commented Sep 8, 2021

Thanks @matthieusieben! Is there any way you could write a test for this?

@matthieusieben
Copy link
Contributor Author

matthieusieben commented Sep 21, 2021

Hey @brianc I added a test that indeed fails on my setup (without the fix of course). Not sure this test is a 100% useful as I don't know if it will fail in every setup, and I don't know how to trigger the same error in other setups...

@brianc
Copy link
Owner

brianc commented Oct 19, 2021

Hey @brianc I added a test that indeed fails on my setup (without the fix of course). Not sure this test is a 100% useful as I don't know if it will fail in every setup, and I don't know how to trigger the same error in other setups...

ah sweet I'll try it over here & see if I can repro. Either way this LGTM so I'll get it merged & released this week. Sorry for the delay. ❤️

@brianc
Copy link
Owner

brianc commented Nov 19, 2021

Hey @matthieusieben - I was gonna merge this today. CI was broken. I fixed it...would you be able to rebase your branch on to current HEAD of my master branch? I did so locally & ran the tests & a pg-query-stream error recovery test broke.

@matthieusieben
Copy link
Contributor Author

Hey @brianc , my wife just gave birth which is why I didn't answer earlier.

I rebased & fixed the issue. The problem was due to the ordering of the operation in the _closePortal method that was preventing this.connection.sync() from being executed.

Should be good now.

@matthieusieben
Copy link
Contributor Author

CI still seem broken but I don't think its related to these changes:

connection-timeout-tests.js
  successful connection ✔
  expired connection timeout Message: listen EADDRINUSE: address already in use 127.0.0.1:54322
Error: listen EADDRINUSE: address already in use 127.0.0.1:54322
    at Server.setupListenHandle [as _listen2] (net.js:1280:14)
    at listenInCluster (net.js:1328:12)
    at GetAddrInfoReqWrap.doListen [as callback] (net.js:1461:7)
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:61:10)

@matthieusieben
Copy link
Contributor Author

RE-running CI worked:
https://github.com/matthieusieben/node-postgres/actions/runs/1507064878

There might be a race condition making the test suite fail ;-)

@matthieusieben
Copy link
Contributor Author

@brianc any change this gets merged any time soon ?

@brianc
Copy link
Owner

brianc commented Jan 28, 2022

ah right on, lemme let the tests run & I'll get this integrated!

@brianc brianc merged commit 5508c0e into brianc:master Jan 28, 2022
@matthieusieben matthieusieben deleted the patch-2 branch January 30, 2022 08:29
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.

Cursor closed twice when used in async iterable
2 participants