-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Prevent lingering MJPEG streams by switching to mode=single when stopping monitor streams #5029
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
Draft
IgorA100
wants to merge
1
commit into
ZoneMinder:master
Choose a base branch
from
IgorA100:patch-483073
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+6
−2
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. You just killed zms. This is stop(), not kill().
stop() means stop streaming, wait for further commands.
Ideally we should be able at this time to tell it to switch to the other monitor and stream from there without relaunching another process but I don't know if that has ever been tried.
Also, we have seen that sometimes zms doesn't get SIGPIPE, so it might linger, preventing the mode=single from happening. You would at least need to remove the connkey. You don't need a connkey if mode=single because it will just exit after the single jpeg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm currently trying to resolve a problem, and it seems there really is a problem with ZMS stopping even when sending a STOP command.
I have a camera with an H.265 stream, go2RTC and rtsp2Web enabled, and the player selection in the monitor settings is set to Auto. We're trying to watch it in Firefox with the player selection set to Automatic, and eventually have to switch to viewing using ZMS. If the player loop is 5 seconds, then between the 4th and 5th seconds, the player cycle reaches ZMS, and then switches to another monitor. At this point, sometimes (probably after the 10th attempt), the stream doesn't stop correctly, and network traffic continues. Something else is happening in the browser, as after about an hour, memory consumption increases, significantly increasing the load on the SDD on the workstation where the browser is running.
If the loop is set to 10 seconds, the problem doesn't seem to occur yet, as the STOP command likely works more correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but let's start with the first thing I see that is wrong: CMD_STOP will not in and of itself kill zms. You want CMD_QUIT.
5s is interesting. I wonder if the signal is coming before we start listening for commands... and so we miss it? Maybe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that this is exactly what is happening. Neither CMD_QUIT, nor mode=single, nor deleting conkey, nor clearing src helps. I've been trying to solve this problem at the JS level for several days now, but so far without success. I can't 100% determine the cause of the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IgorA100 @connortechnology I went digging on this. Isaac's hunch is right — the command does get missed — but the reason it's permanently missed is on the JS side, and it explains why none of
CMD_QUIT/mode=single/ clearingconnkey/ clearingsrcmade any difference.The command isn't lost, the process is
web/js/MonitorStream.js:1383-1394, the failure branch ofgetStreamCmdResponse:On any stream-command error we mint a new connkey and reload the src. And
ajaxError()(web/includes/functions.php:1794) returns HTTP 200 withresult: 'Error', so these land in jQuery's.done(), not.fail()— every one of them takes this path:Socket zms-NNNNNNs.sock does not existTimed out waiting for msgsocket_bind/socket_sendtofailuresThe moment that fires, the still-running zms's connkey is gone from JS.
CMD_STOP,CMD_QUIT,mode=single, clearingsrc— all of them are now addressed to the new connkey. That's why nothing Igor tried helped: the fix is being applied to the wrong process. The orphan can only be stopped by SIGPIPE, which we already know is unreliable.Why the error fires in the first place
closeComms()(src/zm_stream.cpp:410) deliberately never unlinkszms-NNNNNNs.sock; the only thing that removes it is theunlink()beforebind()in a later zms that draws the same connkey. So dead socket files accumulate inPATH_SOCKSforever.web/ajax/stream.php:91iswhile (!file_exists($remSockFile) ...). A file left by a dead zms satisfies that immediately, so PHP skips the wait entirely andsocket_sendtogets ECONNREFUSED → error → orphan.genConnKey()isMath.random()*999999, and a 5-second cycle burns ~720 connkeys an hour, so a collision is likely within the first hour or two and near-certain over an evening. That matches "roughly the 10th attempt" and "worse during multi-hour looped viewing" better than a pure startup race does.Two smaller notes on that loop: it's
1000 × usleep(1000)= 1 second, while the comment right above it says "Pi can take up to 3 seconds for zms to start up." And zms itself is clean here —openComms()binds atzm_monitorstream.cpp:532, before the command thread starts at:611, so datagrams that arrive early are buffered by the kernel rather than dropped.Also:
kill()silently disablesstop()MonitorStream.js:848-855setsthis.started = falseand then callsthis.stop()— which early-returns on!this.started(:712). So for the zms pathclearInterval(this.statusCmdTimer)andclearInterval(this.streamCmdTimer)never run, andactivePlayeris never cleared. A pair of orphaned intervals per cycle. (Thebeforeunloadlistener accumulation that used to compound this is already handled bymanageEventListenersince #5018.)One more, found on the way
StreamBase::lock_fdis initialised to0(src/zm_stream.h:204) while every other use treats negative as "no lock held" —openComms()sets-1on failure,closeComms()guards onlock_fd >= 0. So aStreamBasedestructed withoutopenComms()having succeeded callsclose(0)and closes stdin. It needsconnkey > 0plus arunStream()that returns beforeopenComms(), andMonitorStream::runStream()has two of those: theSTREAM_SINGLEbranch and the!monitorbranch.mode=singleURLs still carry a connkey (:307), so both are reachable.zms exits shortly after and does little in between, so the practical impact today is small — but it's closing a descriptor the class doesn't own, and once fd 0 is free the next
open()silently lands on it.On this PR
I don't think
mode=singleis wrong — it does release the browser-side MJPEG connection, which is real. But it can't help the case Igor is actually hitting, because by then JS is talking to a different connkey than the leaking zms. Worth fixing the ordering first:closeComms()unlink the socket, sofile_exists()means something.kill()/stop()ordering.lock_fdto-1.I have 2, 3 and 4 written up as small independent commits (4 with a regression test; full suite passes at 117 cases / 1788 assertions), happy to open them separately. 1 I'd rather agree on first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IgorA100 I think we found it. #5038 has the fix — it would be very helpful if you could test it against the setup you described (H.265 + go2rtc + rtsp2Web, player on Auto, 5 second loop).
Short version: you were right that the command was being missed, and right that it wasn't fixable from the JS stop path. The reason nothing you tried worked is that by the time you sent
CMD_QUIT/mode=single/ cleared the connkey, the JS was already addressing a different connkey than the zms that was leaking. The fix was being applied to the wrong process.What happens on your 5 second loop:
ZM_WEB_AJAX_TIMEOUT/2(5s by default).stream.phphits theselect()timeout.ajaxErroris commented out there, so it carries on tosocket_recvfrom()on a socket it just set non-blocking. That returnsfalse, andfalse == 0underswitch's loose comparison, so it lands oncase 0and reports'No data to read from socket'.ajaxError()returns HTTP 200 withresult: 'Error', so it arrives in jQuery's.done(), not.fail().getStreamCmdResponse()'s error branch mints a new connkey and reloads the src.So a merely slow zms was being reported exactly like a dead one, then orphaned. That matches what you saw: fine at a 10s loop, failing intermittently at 5s, and getting worse over hours as the orphans accumulate.
The fix classifies each failure in
stream.php(no_socket/timeout/transient/invalid) and only restarts the stream forno_socket. A timeout now just retries on the next poll. And before the connkey is ever replaced,CMD_QUITgoes to the old one first, so a process we're about to lose track of is asked to exit.What's worth checking on your setup:
zmsstill accumulate over a long looped session?ps aux | grep zmsafter an hour or two of 5s cycling.$PATH_SOCKSshould also stop filling withzms-*s.sockfiles, from fix: unlink the zms command socket on exit #5034.On this PR (#5029):
mode=singleisn't wrong — it does release the browser-side MJPEG connection. But it couldn't fix what you were chasing, because the leaking zms was already unaddressable by then. Worth re-testing on top of #5038 to see whether it still helps once the orphaning is gone.Note I haven't tested #5038 against a live install — it's unit tested only, so your real-world check is the one that matters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@connortechnology There are actually many more things that need to be checked. I spent over five days, many hours, experimenting. When cycling through players and then stopping streams, JS would sometimes regularly send packets via this.streamCmdQuery(). But even if I stopped sending CMD_QUERY, the ZMS process wasn't killed, and I don't think this should happen; it's very dangerous.
I'll definitely check out #5038 and let you know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@connortechnology
I tried to compile ZM from the branch https://github.com/connortechnology/ZoneMinder/tree/fix-connkey-regeneration which, as I understand, includes #5038 but the problem remained. After several cycles, the zms process sending the stream to the browser remains, and the browser receives it.