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

Set systemd Type to simple instead of forking #764

Merged

Conversation

fliphess
Copy link
Contributor

@fliphess fliphess commented Feb 8, 2022

Hello :)

I've seen several issues related to webcamd restarting very frequently and ran in the same issue:

The unit webcamd.service completed and consumed the indicated resources.
Feb 08 20:34:56 octopi systemd[1]: webcamd.service: Scheduled restart job, restart counter is at 5.

This mention in the community fixed it for me and as I didn't see it in this branch yet, a PR to change this behaviour.

Hope you are willing to merge this.

Thanks a lot for maintaining this repository :) Keep up the good work!

@cp2004
Copy link
Contributor

cp2004 commented Feb 8, 2022

Is this still required with the changes that were made in #741?

Have you tested a recent nightly build to see if you still have a restarting issue?

@fliphess
Copy link
Contributor Author

fliphess commented Feb 8, 2022

hey @cp2004,

Thanks for your reply!

Is this still required with the changes that were made in #741?
Have you tested a recent nightly build to see if you still have a restarting issue?

I'm not using the latest version, it's a bullseye nightly build from november so it's from after #741 , but I can test it tomorrow with the most recent ones to verify if you want

@guysoft
Copy link
Owner

guysoft commented Feb 8, 2022

It would help if you can test and confirm. It looks though like its a good idea in general.

@fliphess
Copy link
Contributor Author

fliphess commented Feb 8, 2022

I'm printing atm and my job is about to be ongoing for a few hours but right after that I'll give it a try with the latest nightly build to verify.

To be continued :)

@cp2004
Copy link
Contributor

cp2004 commented Feb 8, 2022

hey @cp2004,

Thanks for your reply!

Is this still required with the changes that were made in #741?
Have you tested a recent nightly build to see if you still have a restarting issue?

I'm not using the latest version, it's a bullseye nightly build from november so it's from after #741 , but I can test it tomorrow with the most recent ones to verify if you want

The way I read #741 / #740 was that it was fixing a regular restarting issue, so it is interesting to see that doesn't work.

@fliphess
Copy link
Contributor Author

fliphess commented Feb 9, 2022

The way I read #741 / #740 was that it was fixing a regular restarting issue, so it is interesting to see that doesn't work.

@cp2004: Looking at the commits in #741 those are changes to fix restarts caused by issues at daemon level (IE the webcamd script itself to fix crashes of mjpg_streamer)...
The change I suggest is not: It's on another level to fix a restart that is caused by systemd after a certain time if the script did not fork (I think its 1 minute by default).

When I run:

LD_LIBRARY_PATH=/opt/mjpg-streamer /opt/mjpg-streamer/mjpg_streamer

... And when I run /root/bin/webcamd itself, the script runs in the foreground because the --background or -b argument is not used when starting mjpg_streamer.

We could leave it at forking, but in that case we should start mjpg_streamer with the --background argument to make sure it forks as expected by systemd.

(Although personally I think setting it so Simple is an easier change as forking daemons are not so common anymore since sysv-init has been replaced by systemd)

btw: I've tested with the latest nightly and I still see the same behaviour: My camera runs for around a minute until the streams stops shortly and if I refresh my page it's working again (iow: the restart by systemd completed and it keeps running for the next minute)

@foosel foosel mentioned this pull request Jun 16, 2022
@guysoft
Copy link
Owner

guysoft commented Jun 16, 2022

Hey, @foosel tested, merging this

@guysoft guysoft merged commit 6cb38c2 into guysoft:devel Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants