Skip to content

fix(postgres): prevent crash if postgres connection emit multiple errors#15868

Merged
WikiRik merged 5 commits intosequelize:v6from
Hackatosh:v6
Apr 9, 2023
Merged

fix(postgres): prevent crash if postgres connection emit multiple errors#15868
WikiRik merged 5 commits intosequelize:v6from
Hackatosh:v6

Conversation

@Hackatosh
Copy link
Contributor

@Hackatosh Hackatosh commented Mar 28, 2023 •
edited
Loading

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • If a documentation update is necessary, have you opened a PR to the documentation repository?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Does the name of your PR follow our conventions?

Description Of Change

We recently observed multiple crashes due to an unhandled "server conn crashed?" exception originating from the pg library. As best as I've been able to tell, this shouldn't be possible and should have been fixed in this previous PR : #14731

I think that an error might be able to slip up during the connection without the error listener being properly attached or that error can be fired multiple times and is only handled once.
My fix is to attach the listener before trying to connect to the database. Also I change back the .once handler to a .on (this was changed in the PR 14731) to be able to catch multiple errors.

Todos

@Hackatosh Hackatosh mentioned this pull request Mar 28, 2023
Merged
5 tasks
@Hackatosh Hackatosh mentioned this pull request Mar 31, 2023
Open
2 tasks
@ephys
Copy link
Member

ephys commented Apr 1, 2023

According to pg, errors that occur during connect are provided in its callback. It's important for us that the callback handle connection errors, as we wrap them in errors common to all dialects

I wonder if adding the error listener inside of connect, but before resolve would work?

@Hackatosh Hackatosh changed the title fix(postgres): prevent crash if postgres connection fails during conn… fix(postgres): prevent crash if postgres connection emit multiple errors Apr 3, 2023
@Hackatosh
Copy link
Contributor Author

Hackatosh commented Apr 3, 2023

I have read the code handling the connection in pg and I don't see how an error could be emitted before the handler is attached, so I don't think it's necessary to attach the handler before calling connect()

But I do think that the pg connection can emit multiple errors and that is what is causing our crash, that's why I changed the once to on. Does it seems OK to you ?

@WikiRik WikiRik added the v6 label Apr 3, 2023
@evanrittenhouse
Copy link
Member

evanrittenhouse commented Apr 6, 2023

LGTM - will wait for @ephys or @WikiRik to confirm, since they may have additional insight into to the Postgres error emission logic

@evanrittenhouse evanrittenhouse requested a review from ephys April 6, 2023 14:13
WikiRik
WikiRik approved these changes Apr 8, 2023
View reviewed changes
Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, putting it back to on seems fine to me

@WikiRik WikiRik merged commit 58576dd into sequelize:v6 Apr 9, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 9, 2023

🎉 This PR is included in version 6.31.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@WikiRik WikiRik WikiRik approved these changes

@evanrittenhouse evanrittenhouse evanrittenhouse approved these changes

@ephys ephys Awaiting requested review from ephys

Assignees

No one assigned

Labels

released v6

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Hackatosh @ephys @evanrittenhouse @WikiRik