Only write if file doesn't exist #1226
Conversation
|
I ran these tests locally and they didn't fail. I'm not sure what's going on here :( |
|
Thanks! Tests on master were failing, they've been fixed. Can you pull from master please? I'm wondering if we want to better support the case of multiple servers running from the same directory at the same time. We could possibly add the port to the path of the PID file, though that might break anyone's existing tooling. So in this current PR all servers will be restarted if we touch the You are also correct there are no tests here. We should add some. I'm thinking an integration style test where we have a config file and actually boot up a server or several servers. I have a lib I wrote for integration testing such things https://github.com/schneems/wait_for_it, though Puma already has a few tests that boot servers. We should probably re-visit them sometime as it's relatively easy for them to be left in a bad state and need to be killed manually. |
Before this fix if you have an application that uses Puma and this plugin it is impossible to start 2 servers at once even if they are using separate pidfiles and ports. The reason is because this plugin will overwrite the pidfiles no matter. To reproduce: Start a regular Rails server which points to the pidfile in `tmp/pids/server.pid` ``` rails s ``` Start a second Rails server which points to a new pidfile and port ``` rails s -p 4000 -P tmp/pids/new_server.pid ``` The first server will be stopped and claim that the pid is being used by another process. What really is happening is the restart plugin is writing over the original pidfile which stops the server. If we check the file exists first this will prevent the problem from occuring. Touching tmp/restart.txt will still work but starting a separate process with it's own pidfile should not interfere with other running servers in the same app.
eileencodes
force-pushed
the
only-write-file-if-it-exists
branch
from
65800b0
to
bebdf26
Compare
|
Hey @schneems I rebased with master. I also double checked that yes touching the restart file still restarts both Rails servers. |
Before this fix if you have an application that uses Puma and this
plugin it is impossible to start 2 servers at once even if they are
using separate pidfiles and ports. The reason is because this plugin
will overwrite the pidfiles no matter.
To reproduce:
Start a regular Rails server which points to the pidfile in
tmp/pids/server.pidStart a second Rails server which points to a new pidfile and port
The first server will be stopped and claim that the pid is being used by
another process.
What really is happening is the restart plugin is writing over the
original pidfile which stops the server.
If we check the file exists first this will prevent the problem from
occuring. Touching tmp/restart.txt will still work but starting a
separate process with it's own pidfile should not interfere with other
running servers in the same app.
I really want to write a test for this but I for the life of me cannot figure how to do that. It doesn't look like this plugin has tests and I'm happy to add tests with a little guidance.