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: allow stopping services in "starting" state #503

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

IronCore864
Copy link
Contributor

@IronCore864 IronCore864 commented Sep 24, 2024

Pebble has a 1-second okayWait time period for starting services, after which, if the service is still not exited, it's considered as "running". Previously, when services were still in the starting state within this 1-second okayWait period, they couldn't be stopped. This causes some issues in tests. And, just because services have just started doesn't mean they shouldn't be allowed to stop. This PR allows services in the starting state (within the 1s okayWait period) to be stopped, no matter if it's the daemon exits (Ctrl+C in the console) or the pebble stop command is issued.

Fixes #410
Fixes #502

Also adding two test cases:

  1. servstate: use a mock service which sleeps a while then create a side effect, stop the service immediately after it's started, and the side effect won't be created.
  2. daemon: start a service, stop the daemon before the okay delay, and a stop change should be created. Whether the service is actually terminated or not isn't tested because it's tested above in the servstate.

@IronCore864 IronCore864 changed the title [WIP] fix: allow stopping services that are starting and restart services that quit quickly. fix: allow stopping services that are starting and restart services that quit quickly Sep 26, 2024
@IronCore864 IronCore864 marked this pull request as ready for review September 26, 2024 05:49
@benhoyt benhoyt changed the title fix: allow stopping services that are starting and restart services that quit quickly fix: allow stopping services in "starting" state Sep 26, 2024
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

I'm good with the fix, thanks. However, I wonder if we can spend some time on test cases. Ideally we could have a test for each of the issues this will close:

  • If a service is stopped during okayWait, it 1) returns the "stopped before the okay delay" error, and 2) is actually terminated
  • If the daemon is stopped during okayWait, the service is terminated

internals/overlord/servstate/state-diagram.dot Outdated Show resolved Hide resolved
internals/overlord/servstate/handlers.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants