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

jetty.sh start process should remove jetty_state whenever deleting the pid #7182

Closed
ianrifkin opened this issue Nov 30, 2021 · 2 comments · Fixed by #7184
Closed

jetty.sh start process should remove jetty_state whenever deleting the pid #7182

ianrifkin opened this issue Nov 30, 2021 · 2 comments · Fixed by #7184

Comments

@ianrifkin
Copy link
Contributor

ianrifkin commented Nov 30, 2021

Jetty version(s)
9.4.x
10.x
11.x

In the stop process right after the JETTY_PID file is deleted it will delete the JETTY_STATE file, but this does not happen in the running() routine when it will only delete the PID and not JETTY_STATE if there is an orphaned pid file.

Why this matters: You can end up starting jetty with an outdated JETTY_STATE file. If the contents in JETTY_STATE say "STARTED" it will incorrectly report the JETTY has started before it has. But perhaps worse, if the contents in JETTY_STATE are "STOPPED" or "FAILED" it will report jetty as having failed even if the start is successful.

This is particularly a problem when relying on a service to issue the start command because if jetty.sh reports the start as having failed then it will kill the pid it just started, even though it started fine.

Currently the only way to resolve this situation is to either start then stop with the jetty.sh script to get rid of the jetty_state file or to manually delete the jetty.state file. You cannot fix this relying on the service calling jetty.sh.

My suggestion is a small tweak to jetty.sh. In the case for start it already checks if there's a current PID running using the running() routine. If I'm reading it correctly, when there is a PID file but the PID isn't a running process it will delete the PID file. After this rm statement it should have an additional rm statement to delete the JETTY_STATE file.

With this change in place it should avoid the situation I described. Note that I am not getting into any underlying reasons for why one would be in the situation to trigger this state, but I think it makes sense that if you're deleting the pid file to also delete the state file, to keep things in sync.

Edit: I labeled this an enhancement as I didn't think it was worthy of being a bug, but reflecting on it I think this is probably more like a low priority bug.

ianrifkin added a commit to ianrifkin/jetty.project that referenced this issue Dec 1, 2021
…iously only deleted the pid file.

Signed-off-by: Ian Rifkin <ianrifkin@ianrifkin.com>
@ianrifkin
Copy link
Contributor Author

ianrifkin commented Dec 1, 2021

If you agree with this bugfix, I have submitted a PR for 9.4.x (#7186) and 10.x (#7184) and 11.0.x (#7193). If you disagree with this or want to take a different approach, no worries! Thanks! -Ian

EDIT: I closed the PRs for 10.x and 11.x since I learned I only need one PR for this! Oops! Thanks again.

ianrifkin added a commit to ianrifkin/jetty.project that referenced this issue Dec 1, 2021
…iously only deleted the pid file.

Signed-off-by: Ian Rifkin <ianrifkin@ianrifkin.com>
@ianrifkin
Copy link
Contributor Author

My proposed solution above is just consistently deleting the state file when we delete the pid. But another approach is instead change the lines that grep the state file to instead tail -1 the file then grep that -- because really I think we should only care about what the last line says, this way avoiding the whole issue of if the state file wasn't deleted. Let me know your preference.

ianrifkin added a commit to ianrifkin/jetty.project that referenced this issue Dec 2, 2021
…stently to simply grep'ing for last line in JETTY_STATE file

Signed-off-by: Ian Rifkin <ianrifkin@ianrifkin.com>
joakime pushed a commit that referenced this issue Mar 23, 2022
…y only deleted the pid file. (#7184)

* Issue #7182 changing approach from deleting the state file consistently to simply grep'ing for last line in JETTY_STATE file

Signed-off-by: Ian Rifkin <ianrifkin@ianrifkin.com>
joakime pushed a commit that referenced this issue Oct 6, 2022
…y only deleted the pid file. (#7184)

* Issue #7182 changing approach from deleting the state file consistently to simply grep'ing for last line in JETTY_STATE file

Signed-off-by: Ian Rifkin <ianrifkin@ianrifkin.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment