Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Messenger] Fix exiting messenger:failed:retry command #50787

Merged
merged 1 commit into from
Sep 29, 2023
Merged

[Messenger] Fix exiting messenger:failed:retry command #50787

merged 1 commit into from
Sep 29, 2023

Conversation

HypeMC
Copy link
Contributor

@HypeMC HypeMC commented Jun 27, 2023

Q A
Branch? 6.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

#49539 introduced a bug where it's impossible to exit the messenger:failed:retry command:

Screenshot

Ctrl+C doesn't work because the StopWorkerOnSignalsListener handles the signal but doesn't actually exit the command, so the only way to currently exit the command is to kill it by force.

@carsonbot carsonbot added this to the 6.3 milestone Jun 27, 2023
@HypeMC HypeMC force-pushed the fix-exiting-failedmessagesretrycommand branch from 2627b1b to e5ddf86 Compare June 27, 2023 02:41
@HypeMC HypeMC force-pushed the fix-exiting-failedmessagesretrycommand branch from e5ddf86 to 54c97a4 Compare July 11, 2023 20:11
@HypeMC HypeMC mentioned this pull request Jul 11, 2023
Merged
This was referenced Jul 11, 2023
Merged
Open
lyrixx
lyrixx reviewed Jul 12, 2023
View reviewed changes
Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

I like it.

  1. Could you deprecate the listener
  2. And the service associated with
  3. Add a note in the changelog
  4. Add a note in the upgrade guide?

Thanks

@lyrixx lyrixx mentioned this pull request Jul 13, 2023
Merged
fabpot added a commit that referenced this pull request Jul 13, 2023
@fabpot
bug #50944 [FrameworkBundle] Add missing monolog channel tag to the `…
f57faf3 
…messenger:failed:retry` command (HypeMC)

This PR was merged into the 5.4 branch.

Discussion
----------

[FrameworkBundle] Add missing monolog channel tag to the `messenger:failed:retry` command

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Noticed this while working on #50787, similar to #49843.

Commits
-------

8f30c1e [FrameworkBundle] Add missing monolog channel tag to the `messenger:failed:retry` command
nicolas-grekas added a commit that referenced this pull request Jul 13, 2023
@nicolas-grekas
bug #50945 [DebugBundle][FrameworkBundle] Fix using the framework wit…
97bd5a0 
…hout the Console component (HypeMC)

This PR was merged into the 5.4 branch.

Discussion
----------

[DebugBundle][FrameworkBundle] Fix using the framework without the Console component

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

While working on #50787 I've noticed that it's impossible to use the framework without the Console component, event though it's an optional dependency.
This PR aims to fix that. I've tested these changes on 5.4 & 6.3, however, I only made sure the container can compile. I'm not sure if there are any other gotchas.

Commits
-------

feddf40 [DebugBundle][FrameworkBundle] Fix using the framework without the Console component
@HypeMC HypeMC force-pushed the fix-exiting-failedmessagesretrycommand branch from 54c97a4 to f76fd88 Compare July 13, 2023 17:16
@HypeMC
Copy link
Contributor Author

HypeMC commented Jul 13, 2023

I like it.

  1. Could you deprecate the listener
  2. And the service associated with
  3. Add a note in the changelog
  4. Add a note in the upgrade guide?

Thanks

@lyrixx Done

@HypeMC HypeMC mentioned this pull request Jul 16, 2023
Merged
@HypeMC HypeMC force-pushed the fix-exiting-failedmessagesretrycommand branch from f76fd88 to 74b5ff1 Compare July 16, 2023 22:54
nicolas-grekas added a commit that referenced this pull request Jul 19, 2023
@nicolas-grekas
feature #50997 [Messenger] Deprecate StopWorkerOnSignalsListener (H…
40d61eb 
…ypeMC)

This PR was merged into the 6.4 branch.

Discussion
----------

[Messenger] Deprecate `StopWorkerOnSignalsListener`

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | -
| License       | MIT
| Doc PR        | -

Followup to #50787.

Commits
-------

0b62ce8 [Messenger] Deprecate `StopWorkerOnSignalsListener`
@HypeMC HypeMC mentioned this pull request Jul 24, 2023
Merged
derrabus added a commit that referenced this pull request Jul 25, 2023
@derrabus
minor #51093 [FrameworkBundle] Fix using messenger 7.0 (HypeMC)
5df1aba 
This PR was merged into the 6.4 branch.

Discussion
----------

[FrameworkBundle] Fix using messenger 7.0

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Makes sure messenger v7.0 wouldn't break, see #51064 (comment). Could be update for #50787 (comment) as well.

Commits
-------

9a45ae0 [FrameworkBundle] Fix using messenger 7.0
@HypeMC HypeMC force-pushed the fix-exiting-failedmessagesretrycommand branch from 74b5ff1 to 8cf7139 Compare July 31, 2023 15:54
@HypeMC HypeMC force-pushed the fix-exiting-failedmessagesretrycommand branch from 8cf7139 to 5f45cef Compare September 5, 2023 18:52
@HypeMC
Copy link
Contributor Author

HypeMC commented Sep 5, 2023

@lyrixx @xabbuh Hi guys, is there anything else I can do here, this bug is really becoming a nuisance so it'd be great if it got fixed 😄

nicolas-grekas
nicolas-grekas approved these changes Sep 29, 2023
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

LGMT, just minor things on my side.

src/Symfony/Component/Messenger/Command/ConsumeMessagesCommand.php Outdated Show resolved Hide resolved
src/Symfony/Component/Messenger/Command/ConsumeMessagesCommand.php Outdated Show resolved Hide resolved
src/Symfony/Component/Messenger/Command/FailedMessagesRetryCommand.php Outdated Show resolved Hide resolved
@HypeMC HypeMC force-pushed the fix-exiting-failedmessagesretrycommand branch from 5f45cef to cd6816b Compare September 29, 2023 10:45
@nicolas-grekas
Copy link
Member

Thank you @HypeMC.

@nicolas-grekas nicolas-grekas merged commit 0539ca8 into symfony:6.3 Sep 29, 2023
9 checks passed
@HypeMC HypeMC deleted the fix-exiting-failedmessagesretrycommand branch September 29, 2023 20:03
@fabpot fabpot mentioned this pull request Sep 30, 2023
Merged
@digilist digilist mentioned this pull request Sep 30, 2023
Closed
@JoppeDC JoppeDC mentioned this pull request Oct 6, 2023
Closed
@Bilge Bilge mentioned this pull request Oct 8, 2023
Closed
@hlecorche hlecorche mentioned this pull request Oct 16, 2023
Closed
@HypeMC HypeMC mentioned this pull request Oct 16, 2023
Merged
fabpot added a commit that referenced this pull request Oct 16, 2023
@fabpot
bug #52080 [Messenger] Fix graceful exit (HypeMC)
8a1fba8 
This PR was merged into the 6.3 branch.

Discussion
----------

[Messenger] Fix graceful exit

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #52077
| License       | MIT

My previous PR #50787 accidentally broke the behavior of the `messenger:consume` command. It no longer waits for the handler to finish, instead it exists immediately.

Commits
-------

b270382 [Messenger] Fix graceful exit
@SerheyDolgushev SerheyDolgushev mentioned this pull request Nov 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@lyrixx lyrixx lyrixx left review comments

@xabbuh xabbuh xabbuh left review comments

Assignees
No one assigned
Labels
Bug Messenger Status: Reviewed
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@HypeMC @nicolas-grekas @fabpot @lyrixx @Bilge @xabbuh @carsonbot