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

Register shutdown function to kill process in the event of an early exit #76

Merged
merged 3 commits into from
Sep 20, 2023

Conversation

joetannenbaum
Copy link
Contributor

I preface this PR by saying I don't love this solution, but I was struggling to find another way to handle this.

There seems to be an issue where if the callback function exits early, the process rendering the spinner continues running:

$result = spin(
    function () {
        exit('Heading out early.');
    },
    'Installing dependencies...',
);
CleanShot.2023-09-19.at.21.12.42.mp4

I'm using register_shutdown_function for the $pid that's not 0, which, if this is the solution, might negate the need for the posix_kill in the resetTerminal method.

Is this something you're interested in handling? If so... is there a better way? I couldn't figure out a way to catch this otherwise.

@jessarcher jessarcher added the bug label Sep 20, 2023
@jessarcher jessarcher self-assigned this Sep 20, 2023
@jessarcher
Copy link
Member

jessarcher commented Sep 20, 2023

Hey @joetannenbaum,

Thanks for this! I think we'll also want to restore the cursor like we do when receiving SIGINT.

I'm also wondering whether we should erase the rendered lines on exit (and potentially even on SIGINT). Edit: This is complicated because the shutdown function will stay for the remainder of execution, including after other prompts have been registered.

@jessarcher jessarcher marked this pull request as draft September 20, 2023 01:57
@jessarcher
Copy link
Member

jessarcher commented Sep 20, 2023

I'm using register_shutdown_function for the $pid that's not 0, which, if this is the solution, might negate the need for the posix_kill in the resetTerminal method.

I think we'll still need this to stop the renderer process when everything is successful.

Edit: I think I see what you mean - we could just call posix_kill after the callback, because any "unhappy path" scenarios would be caught by the shutdown function, negating the need to conditionally check for a PID in scenarios where it's not applicable.

@jessarcher
Copy link
Member

I've made sure the cursor is restored on shutdown in the same way we do with other prompts, where it's a no-op if the cursor is already shown. This is important because we can't unregister the shutdown function.

I've also simplified the killing of the child process from #71 now that your handler will take care of that too. I've tested that your shutdown handler is a no-op when the spinner doesn't error out.

@jessarcher jessarcher marked this pull request as ready for review September 20, 2023 03:38
@joetannenbaum
Copy link
Contributor Author

Good call on the cursor restore, didn't think about that. Glad this worked out!

@taylorotwell taylorotwell merged commit a9a7bb7 into laravel:main Sep 20, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants