-
Notifications
You must be signed in to change notification settings - Fork 106
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
remove application from pool on timeout #175
Conversation
We faced the same issue. |
Thanks for tracking this down! Please add a test. |
fair enough @nateberkopec would you be open to merging #188 ? AFAIK it's necessary to build with
|
Sure, I'll take a look and repro. If I can get the tests running should be a simple merge. |
@nonrational Fixed the dep, Travis should be running soon too. Back to you 🙇 |
Before going wild trying to mock I have two rails applications,
I think run the following on #!/usr/bin/env bash
# reproduce.sh
# clean up and build a brand new puma-dev binary
rm -f ./puma-dev
make
# remove old logs
mv -v ~/Library/Logs/puma-dev.log ~/Library/Logs/puma-dev.log.$(date '+%Y%m%d%H%M%S')
# ensure we don't have a lingering installation against an old binary
./puma-dev -uninstall -d test:localhost
# install with a 5 second timeout to make testing quicker
./puma-dev -d test:localhost -timeout 5s -install
sudo ./puma-dev -d test:localhost -timeout 5s -setup
# wait for daemon to fully init
echo "Sleeping for 5s..." && sleep 5
# curl two different apps
curl http://hello-world.localhost/ > /dev/null
curl http://myapp.localhost/ > /dev/null
# wait for the timeout and a little
echo "Sleeping for 10s..." && sleep 10
# check that both apps received boot and cleanup
cat ~/Library/Logs/puma-dev.log | grep 'booted\|cleaned' and here's the output:
The fact that both applications get the "... and cleaned up" means we got here https://github.com/nonrational/puma-dev/blob/master/dev/app.go#L136-L146 which indicates that the application was correctly cleaned up. I invite others experiencing this issue to try to repro on master. I'm going to try to repro with an older ruby and puma. If I'm unable to reproduce, will close this PR. |
failed to reproduce with
|
failed to reproduce with
closing. 🙌 |
This PR addresses #59.
On successful
SIGTERM
,Kill()
doesn't remove the application from the pool. As a result, on the next request,puma-dev
thinks the app is already running and doesn't boot it. With this patch, we ensure thatpuma-dev
boots a new puma process on the next request.