Skip to content

[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 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 5 commits into from
Closed

Conversation

J-roen
Copy link
Contributor

@J-roen J-roen commented Dec 15, 2023

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #50833
License MIT

Fixes using negative delays when sending messages.

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 7.1 branch.

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!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

J-roen and others added 3 commits December 15, 2023 20:25
@xabbuh
Copy link
Member

xabbuh commented Dec 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
Copy link
Contributor

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
Copy link
Contributor

valtzu commented Dec 16, 2023

@xabbuh If you have a long queue, you can cut in front of the queue with this negative delay because messages are sorted by available_at.

So it's about priority within a single queue basically.

@J-roen
Copy link
Contributor Author

J-roen commented Dec 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
Copy link
Member

I'm not certain on this because:

  • we should never expect things based on bridge implementation details
  • i'm not certain it really "works" with this PR (everytime for everyone i mean).. I think about postgres notify behaviour, hard indexes on dbs... So this would need a loot of tests .. it this worth the effort ?

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
Copy link
Contributor Author

J-roen commented Dec 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
Copy link
Member

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.

I totally agree!

@fabpot fabpot modified the milestones: 6.4, 5.4 Dec 19, 2023
@fabpot
Copy link
Member

fabpot commented Dec 19, 2023

@J-roen Can you create a PR for Symfony 5.4, which is also affected?

@J-roen
Copy link
Contributor Author

J-roen commented Dec 19, 2023

@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?

Copy link
Member

@Nyholm Nyholm left a 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.

fabpot added a commit that referenced this pull request Dec 24, 2023
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       | MIT

Fixes using negative delays when sending messages. Cherry-picked from #53088.

Commits
-------

af1517e [Messenger] Fix using negative delay
@fabpot fabpot closed this Dec 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants