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.

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

Use trap method instead of pcntl_signal for better signal handling #6

Closed
wants to merge 1 commit into from
Closed

Use trap method instead of pcntl_signal for better signal handling #6

wants to merge 1 commit into from

Conversation

Thavarshan
Copy link

Description

This PR replaces the direct use of pcntl_signal with the trap method provided by the InteractsWithSignals trait in the TopCommand class. The trap method allows for multiple handlers for the same signal, preventing potential issues in environments like Laravel Octane where multiple handlers might be necessary.

Changes

  • Replaced pcntl_signal(SIGINT, function () use ($guiBuilder) { ... }); with $this->trap(SIGINT, function () use ($guiBuilder) { ... });

Benefits

  • Supports multiple handlers for signals, ensuring that no handlers are overwritten.
  • Provides better integration with Laravel's command lifecycle and signal management.
  • Enhances the robustness and reliability of signal handling in the application.

Testing

  • Tested the changes locally to ensure that the signal handling works as expected.
  • Verified that the application exits alternate screen mode and shows the cursor upon receiving SIGINT.

@marcioal1991
Copy link

This closes the #4 issue.

Cheers!

@leventcz leventcz self-requested a review May 28, 2024 19:08
@leventcz
Copy link
Owner

Thank you for your contribution.

I like this approach and want to merge it, but it's not working for me right now.

  • Verified that the application exits alternate screen mode and shows the cursor upon receiving SIGINT.

I'm using Docker with PHP 8.2 and have PCNTL enabled. Somehow, it is not triggering the callback.

I didn't have this issue with the previous version.

I'll investigate further.

@leventcz leventcz linked an issue May 28, 2024 that may be closed by this pull request
@leventcz leventcz linked an issue May 28, 2024 that may be closed by this pull request
@Thavarshan Thavarshan closed this by deleting the head repository Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use of the pnctl_signal function directly
3 participants