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] Remove fixNoAutoAckStamp handling inWorker::flush()#61291
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
Worker::flush() methodThis PR not only removes unused code, but changes code. Can you please add or modify an existing test case? Thanks |
1d1766d tobb580f1CompareWorker::flush() methodNoAutoAckStamp handling inWorker::flush()NoAutoAckStamp handling inWorker::flush()NoAutoAckStamp handling inWorker::flush()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.
The fix does make sense to me.
Ideally the test case shouldn't use reflection to hack into internal structures.
Would it be possible to use only public methods instead?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
NoAutoAckStamp handling inWorker::flush()NoAutoAckStamp handling inWorker::flush()bb580f1 to1d3caf6CompareThat's the best I could come up with. Using only minimal Reflection to fill the |
The line `$envelope = $envelope->withoutAll(NoAutoAckStamp::class);` was modifying a local variable that was never used after assignment. This change moves the stamp removal to the catch block where it's actually needed, ensuring envelopes stored for acknowledgment don't contain contradictory NoAutoAckStamp.
1d3caf6 to06b1d8fCompareThank you@wazum. |
9aa072c intosymfony:6.4Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Description
This PR removes unused code in the
Worker::flush()method where an envelope modification was never used.Changes
$envelope = $envelope->withoutAll(NoAutoAckStamp::class);which was modifying a local variable that was immediately discardedNoAutoAckStampremoval to the catch block where it's actually neededWhy this change?
The line was unused code because:
dispatch()succeeds: The modified envelope is never used (foreach continues/ends)dispatch()throws: The line is never reachedMoving the stamp removal to the
catchblock ensures that envelopes stored for acknowledgment have clean state - they shouldn't contain a "no auto ack" stamp when they're about to be acknowledged.