Prevent lingering MJPEG streams by switching to mode=single when stopping monitor streams - #5029
Prevent lingering MJPEG streams by switching to mode=single when stopping monitor streams#5029IgorA100 wants to merge 1 commit into
Conversation
…ping monitor streams
| if (this.started || wasStarting) this.streamCommand(CMD_STOP); | ||
| const src = stream.src; | ||
| stream.src = ""; | ||
| stream.src = src.replace(/([?&])mode=[^&]*/, '$1mode=single'); |
There was a problem hiding this comment.
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.
Also, we have seen that sometimes zms doesn't get SIGPIPE, so it might linger,
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.
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.
I wonder if the signal is coming before we start listening for commands... and so we miss it? Maybe.
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.
@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 / clearing connkey / clearing src made any difference.
The command isn't lost, the process is
web/js/MonitorStream.js:1383-1394, the failure branch of getStreamCmdResponse:
} else {
if (!this.started) return;
console.error(respObj.message);
// Try to reload the image stream.
...
this.streamCmdParms.connkey = this.statusCmdParms.connkey = this.connKey = this.genConnKey();
src = src.replace(/connkey=\d+/i, 'connkey='+this.connKey);
stream.src = ''; stream.src = src;
}On any stream-command error we mint a new connkey and reload the src. And ajaxError() (web/includes/functions.php:1794) returns HTTP 200 with result: '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_sendtofailures
The moment that fires, the still-running zms's connkey is gone from JS. CMD_STOP, CMD_QUIT, mode=single, clearing src — 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 unlinks zms-NNNNNNs.sock; the only thing that removes it is the unlink() before bind() in a later zms that draws the same connkey. So dead socket files accumulate in PATH_SOCKS forever.
web/ajax/stream.php:91 is while (!file_exists($remSockFile) ...). A file left by a dead zms satisfies that immediately, so PHP skips the wait entirely and socket_sendto gets ECONNREFUSED → error → orphan. genConnKey() is Math.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 at zm_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 disables stop()
MonitorStream.js:848-855 sets this.started = false and then calls this.stop() — which early-returns on !this.started (:712). So for the zms path clearInterval(this.statusCmdTimer) and clearInterval(this.streamCmdTimer) never run, and activePlayer is never cleared. A pair of orphaned intervals per cycle. (The beforeunload listener accumulation that used to compound this is already handled by manageEventListener since #5018.)
One more, found on the way
StreamBase::lock_fd is initialised to 0 (src/zm_stream.h:204) while every other use treats negative as "no lock held" — openComms() sets -1 on failure, closeComms() guards on lock_fd >= 0. So a StreamBase destructed without openComms() having succeeded calls close(0) and closes stdin. It needs connkey > 0 plus a runStream() that returns before openComms(), and MonitorStream::runStream() has two of those: the STREAM_SINGLE branch and the !monitor branch. mode=single URLs 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=single is 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:
- Don't regenerate the connkey on error without dealing with the old zms first. This is the actual root cause and it's a design call — @connortechnology, is reusing the connkey on retry acceptable once the socket file is a reliable liveness signal?
- Make
closeComms()unlink the socket, sofile_exists()means something. - Fix the
kill()/stop()ordering. - Initialise
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.
@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:
- A stream command to zms doesn't get a reply within
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.- The original zms is still alive and streaming, but now unreachable — nothing addresses its connkey any more. Only SIGPIPE can stop it, and we already know that's unreliable.
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 for no_socket. A timeout now just retries on the next poll. And before the connkey is ever replaced, CMD_QUIT goes 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:
- Does
zmsstill accumulate over a long looped session?ps aux | grep zmsafter an hour or two of 5s cycling. - Does the browser memory growth stop? That was the symptom loading your workstation's SSD.
$PATH_SOCKSshould also stop filling withzms-*s.sockfiles, from fix: unlink the zms command socket on exit #5034.
On this PR (#5029): mode=single isn'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.
@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.
@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.
|
Uh... reading the comments in that function... apparently at some point stop() really became killing the stream. In which case we should be sending QUIT instead of STOP. the mode=single should not be necessary. |
|
|
Yes to all. I believe we added support for the first but you have to specify it. Should double check. |
|
I tried not sending CMD_QUERY commands, but the browser still receives the stream. |
|
@connortechnology |
|
But how do we know how long the ZMS should last if no requests are received from the browser? |
lock_fd was initialised to 0, but every other use in the class treats a negative value as "no lock held": openComms() sets it to -1 when open() or flock() fails, and closeComms() guards on `lock_fd >= 0` before closing. The constructor's 0 passes that guard, so a StreamBase destructed without openComms() having succeeded calls close(0) and closes stdin. Reaching it only takes connkey > 0 plus a runStream() that returns before openComms(), and MonitorStream::runStream() has two such returns: the STREAM_SINGLE branch and the !monitor branch. mode=single URLs still carry a connkey, so both are reachable in normal operation. zms exits shortly afterwards and does little in between, so the practical impact today is small. It is still closing a descriptor the class does not own, and once fd 0 is free the next open() in the process silently lands on it. Add a regression test covering destruction with and without a connkey. It saves and restores fd 0 so a failure can't cascade into the rest of the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
I'll put claude to it. I don't really want to add more config options if we can avoid it. Just makes it too complex for people who never read the docs anyways. We could simply use the max value between the bandwidth options as an absolute limit. But the web UI side should specify it in the zms url to give a better value. |
kill() cleared this.started before calling this.stop(), but stop() returns early when !started. For the zms path that meant clearInterval() on statusCmdTimer and streamCmdTimer never ran, activePlayer was never reset and mediaStream/audioTrack/videoTrack were never released. Every kill() leaked a pair of intervals, which adds up over a montage or watch page that cycles monitors every few seconds. Keep started set until stop() has done its work, and pass skipStreamCommand so stop() doesn't follow CMD_QUIT with a CMD_STOP against a socket zms is already tearing down. Clear connkey afterwards. stop() already sets started=false and activePlayer='' at the end, so kill() doesn't need to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
closeComms() left /var/lib/zoneminder/sock/zms-NNNNNNs.sock behind, and the only thing that ever removed it was the unlink() before bind() in a later zms that happened to draw the same connkey. Socket files accumulated indefinitely. web/ajax/stream.php uses file_exists() on that path to decide whether zms is listening, and waits up to a second for it to appear before giving up. A file left by an exited zms defeats that wait: the check passes immediately, the sendto() gets ECONNREFUSED, and the command is reported as failed even though the new zms was about to bind. genConnKey() draws from six digits, so a page cycling monitors every five seconds reuses a key well within an hour. Unlink while we still hold the flock. A second zms with the same connkey blocks on flock(LOCK_EX) in openComms() before it unlinks and binds, so it cannot have created its own socket yet and we can't delete a file belonging to it. The lock file itself is still left alone, since another zms may be waiting on it. This is best effort: zms killed by a signal still leaves the socket behind. In that case the process is usually still running, so the file being there is not wrong. Also reset lock_fd after closing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Ok. When you're done with Claudie, please let me know. |
getStreamCmdResponse() responded to every ajax/stream.php failure the same way: mint a fresh connkey and reload the img src. ajaxError() returns HTTP 200 with result=Error, so these arrive in jQuery's done() rather than fail(), and all twelve error paths in stream.php took that branch. Only one of them means zms is gone. For the rest the process is still running and streaming, and replacing the connkey makes it unaddressable: CMD_STOP, CMD_QUIT and mode=single all then go to the new key, so nothing can reach the old process and only SIGPIPE can stop it, which we know is unreliable. That is why the reports of lingering zms after switching monitors were unaffected by changes to what the stop path sends. The timeout path made this routine rather than rare. On select() expiry ajaxError is commented out, so the script carries on to socket_recvfrom() on a now non-blocking socket. That returns false, and false == 0 under switch's loose comparison, so a merely slow zms was reported as 'No data to read from socket' and torn down. stream.php now classifies each failure as no_socket, timeout, transient or invalid, and sends it as 'reason'. The client restarts the stream only for no_socket. A missing reason is still treated as fatal, so a php that predates this keeps the old behaviour. Before replacing the connkey the client now sends CMD_QUIT to the old one, so the process we are about to lose track of is asked to exit. That is deliberately not routed through streamCommand(): it must name its target explicitly, since this.connKey is about to change, and its response must not feed back into getStreamCmdResponse(), or a QUIT that also failed would re-enter the error path and loop. ajaxError() takes the classification as a third argument, named $reason because $code is already the HTTP status, and only includes it when set, so the other 131 callers are unaffected. Tests: tests/js covers the fatal/non-fatal decision including the no-reason fallback, tests/php pins the classification mapping and the switch(false) semantics the timeout branch depends on. Both verified to fail when the behaviour is reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
When quickly switching monitors, for example using a 5-second loop and automatically selecting players on the Watch page, the stream isn't always completely cleared. This is especially noticeable during multi-hour looped viewing. It's likely that the browser is still holding some streams.
The resource leak for all players except ZMS was fixed in #5018.
Currently, after PR #4894,
mode=singleworks correctly for us, and I see no reason why we can't usemode=single.Perhaps the
streamCommand(CMD_STOP)command will now be redundant, but I didn't remove it because I hope it won't make things worse.@connortechnology I'd like to hear your opinion. You spoke out against killing ZMS processes here #4706, but in this PR we're not killing them; they remain but don't consume CPU resources.