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

Fix #851: When docker stop container, db connections still persists #853

Conversation

Focusnik
Copy link
Contributor

@Focusnik Focusnik commented Mar 15, 2024

fix for #851

Problem:

When we run octane+openswoole in docker container we can't use CTRL+C combination to stop the server.
In docker case, container operation system will be ripped during stop phase. In common cases, docker send SIGTERM signal into bash/sh terminal, that is an owner of the container. But, terminal SIGTERM not propagated into "octane" commands.

Solution:

We should trap SIGHUP (terminal connection disrupted) signal, because only this signal will be sent into "octane" command, when terminal window closed. That can be easily tested on local machine: open OS task manager, find php stack and run "octane:start --server=swoole", and close the terminal process. You will see, that php processes still there. But if we trap SIGHUP signal, octane command will receive the signal to stop the server.

That's it

@driesvints
Copy link
Member

@Focusnik please add a thorough description here and not just link to an issue.

@Focusnik
Copy link
Contributor Author

Focusnik commented Mar 15, 2024

@driesvints done

@Focusnik
Copy link
Contributor Author

Focusnik commented Mar 15, 2024

Also, i figured out some notice for docs mby. When we starting container in docker, we also make a volume for the logs folder. Octane creates server configuration file in the logs folder. So, when we scale docker container into >1 replicas with mounted logs directory we will have json file with pids for latest replica, that wrote into it. Developers must be careful with that and change state_file config value to some none mounted folder, so file will be unique per replica

@taylorotwell taylorotwell marked this pull request as draft March 15, 2024 14:10
Copy link
Member

@nunomaduro nunomaduro left a comment

Choose a reason for hiding this comment

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

I've literally no idea if this will be a regression in the future. I think no; so will propose we merge this @taylorotwell and stay vigilant if anyone complains.

@nunomaduro nunomaduro marked this pull request as ready for review March 28, 2024 14:38
@taylorotwell taylorotwell merged commit a391124 into laravel:2.x Mar 28, 2024
14 checks passed
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.

4 participants