Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add synctl warning when no process is stopped and fix restart #6598

Merged
merged 9 commits into from
May 19, 2020

Conversation

romainbou
Copy link
Contributor

@romainbou romainbou commented Dec 27, 2019

As suggested in #5166, I added a warning when no process is killed for synctl stop and synctl restart
For synctl restart if no process is killed don't attempt to start anyway.

Example stopping synapse twice:

> synctl stop 
stopped synapse.app.homeserver

> synctl stop
No running worker found. (from '~/synapse/homeserver.pid')
The process might be managed by another controller (e.g. systemd)

> synctl restart
No running worker found. (from '~/synapse/homeserver.pid')
The process might be managed by another controller (e.g. systemd)

Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Sorry for the delay in reviewing this! Looks pretty good overall, but I left a few comments.

synctl Outdated Show resolved Hide resolved
synctl Outdated Show resolved Hide resolved
synctl Outdated Show resolved Hide resolved
synctl Outdated Show resolved Hide resolved
synctl Outdated Show resolved Hide resolved
@clokep
Copy link
Member

clokep commented Apr 6, 2020

@romainbou Can you fix the lint issues? You can run them locally with: ./scripts-dev/lint.sh

synctl Outdated Show resolved Hide resolved
@clokep
Copy link
Member

clokep commented May 15, 2020

@romainbou Do you have plans to take a look back at this PR? It'd be great to finish it up!

@clokep
Copy link
Member

clokep commented May 18, 2020

@romainbou Can you merge develop into this branch? There's been some changes to CI and such since the PR was first opened!

@romainbou
Copy link
Contributor Author

@clokep cool thanks yes indeed. After merging develop again, now all checks are passing!

@clokep clokep self-requested a review May 19, 2020 11:32
synctl Outdated Show resolved Hide resolved
changelog.d/6590.misc Outdated Show resolved Hide resolved
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for making those changes! 👍

@clokep clokep merged commit a57863d into matrix-org:develop May 19, 2020
@richvdh
Copy link
Member

richvdh commented Jun 1, 2020

this seems to have had the side-effect of changing the behaviour of synctl restart so that it doesn't start synapse if it wasn't running previously. Was this a deliberate change? IMHO it's somewhat unintuitive and inconsistent with the behaviour of other process managers.

@richvdh
Copy link
Member

richvdh commented Jun 3, 2020

it seems that it was a deliberate change, and indeed that was somewhat suggested by #6994.

On reflection, I feel like the downsides of that change outweigh the upsides (specifically: it's a surprise to anyone who has ever used synctl or any other process manager that synctl restart does not start synapse if one was not previously running). Accordingly, I have reverted that part of the change in #7624. The new warning text is retained.

@richvdh
Copy link
Member

richvdh commented Jun 3, 2020

I should also mention: I don't think that changing the behaviour of synctl restart in this way brought any particular advantages. (The goal was to help people who blindly assumed that the stop part of synctl restart had worked: whether or not we go on to do a start after a failed stop has no bearing on that.)

An even better fix here is to sort out #4641, but for now the warning text is an improvement.

phil-flex pushed a commit to phil-flex/synapse that referenced this pull request Jun 16, 2020
…6598)

* If an error occurs when stopping a process synctl now logs a warning.
* During a restart, synctl will avoid attempting to start Synapse if an error
  occurs during stopping Synapse.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants