Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] Fix using negative delay#53088
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
carsonbot commentedDec 15, 2023
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
src/Symfony/Component/Messenger/Bridge/Doctrine/Tests/Transport/DoctrineIntegrationTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
xabbuh commentedDec 16, 2023
Actually, I do not really understand the use case for using a negative delay. Is that really something that needs to be supported? |
OskarStark commentedDec 16, 2023
As far as I understand, the problem is that it does not work right now, and this can happen due to calculation, so it should "just" send the message right? |
valtzu commentedDec 16, 2023
@xabbuh If you have a long queue, you can cut in front of the queue with this negative delay becausemessages are sorted by So it's about priority within a single queue basically. |
J-roen commentedDec 16, 2023
Actually, it was already possible to set negative delays, but the behaviour was unexpected (in my opinion). Another solution could be to set the time to "now" in case of a negative delay. But then use cases like described by@valtzu wouldn't be supported. |
smnandre commentedDec 17, 2023
I'm not certain on this because:
As i see it, you removed a valid safeguard in the code to allow a "non-written" behaviour.. and i'm pretty sure this will not lead to good things :) And finally... there is a simpler / documented way to handle queue priority:queue priority. On a fonctional level, does this not fill your needs ? |
J-roen commentedDec 17, 2023
I agree that you can discuss about the expected behaviour, but I think that what you at least wouldn't expect, is that a negative delay is converted to a positive delay. How it's supposed to work apart from that, could maybe remain undefined. |
smnandre commentedDec 17, 2023
I totally agree! |
fabpot commentedDec 19, 2023
@J-roen Can you create a PR for Symfony 5.4, which is also affected? |
J-roen commentedDec 19, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yes, I can do that later this week. And what about 7.x? |
Nyholm left a comment
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.
Thank you
@J-roen Can you create a PR for Symfony 5.4, which is also affected?
Yes, I can do that later this week. And what about 7.x?
It is no need to add this to 7.x. When this is merged in 6.4 it will be merge "up" to 7.0 and 7.1 by a maintainer.
This PR was squashed before being merged into the 5.4 branch.Discussion----------[Messenger] Fix using negative delay| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues |Fix#50833| License | MITFixes using negative delays when sending messages. Cherry-picked from#53088.Commits-------af1517e [Messenger] Fix using negative delay
Uh oh!
There was an error while loading.Please reload this page.
Fixes using negative delays when sending messages.