Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
Always attempt to listen for notifications#47209
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
I've also added a test but i'm not sure it is worth the effort |
d2dd112 to5700919CompareIf we're sure that it's safe to call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is mentioned in the LISTEN docs that calling it when psql is already listening, does nothing. So, it looks like it "safe".
@goetas Can you have a look at the tests,https://github.com/symfony/symfony/runs/7711529784?check_suite_focus=true might be related to this change. |
| public function reset() | ||
| { | ||
| parent::reset(); | ||
| $this->unlisten(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
don't we want to keep this? we still want to unlisten on reset, don't we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
i have reverted a portion of this. I have also verified that callingUNLISTEN multiple times makes no harm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
thanks for taking this on !
eaa0b31 tob462665Compare| }; | ||
| // dbal 2.x | ||
| if (interface_exists(Result::class)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I had to do this weird thing to test against dbal 2.x and dbal 3.x. I'm not proud of it but i do not have a better idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You could maybe duplicate the test to remove the if (and skip if not using the good version). But IMHO that's good enough as-is.
I have fixed the test, it was because i had locally dbal 3.x instlled while on CI 2.x is runing.https://github.com/symfony/symfony/pull/47209/files#r939714144 does a different mocking by trying to detect which dbal is running |
b462665 to72ac5bfCompareThank you@goetas. |
…es with custom consumption priorities. Replaysymfony#47209 on top of this code change.
as discussed in#45056 (comment), when a middleware closes the connection, the messenger consumer will not receive notifications when a new record is inserted.
Since is safe to call
LISTENmultiple times, we can avoid having to keep tack if we did call or not the listen command, we call it always so we are always sure to get notifications with new messages are added.