command: make hook removal logging to verbose #15246
Conversation
Hooks are implicitly removed if the mpv_handle it was registered with is destroyed. This is documented in libmpv/client.h and is expected behavior for hook removal, so it should not be treated as a warning. Make it verbose instead. The log level for "client removed during hook handling" is unchanged because the documentation says it should be avoided.
|
I think the warn is good, we should clean up the hooks after scripts, see #15245 You should join IRC one day for those juicy discussions :) |
There is no need to do that. The existing code already works as expected to remove the stale hooks, it's just that the logging is more verbose than it needed. There is no reason to warn this because there is no way to remove hooks other than destroying the mpv client handle. |
It's this cleaner to do explicitly like with input bindings? And not log bogus verbose messages? That mean nothing, because it is expected behavior apparently? Also in perfect world without bugs it maybe is true, but such warning message is clearly saying that we have hooks that are stale. If someone in the future adds remove_hook this would be useful to catch possible errors. EDIT: Also this is pointless loop of send hook notification, fail, wakup core, repeat. Just for the hooks that we explicitly know that should have been removed. |
|
Download the artifacts for this pull request: |
But the warning wasn't about non-existing client. It's about failing to send the message. One reason for it is indeed if the client no longer exists. Another could be that its event queue is full, or maybe other reasons. I think the best approach is to remove the hook once a client exists. I did not look at the code myself, so I don't have an approach for an implementation, but apparently, if I understood correctly (according to @guidocella) if a client exits with some non-success code (e.g. exception in a script) then this warning is not printed. So it might be worth looking into that, and make that happen (no warning) even if the client exited without error. |
|
If you apply only the 2nd commit of #15245 auto_profiles.lua still logs the warning only for on_before_start_file, presumably because of a race condition of that hook triggering at the same as the profile-list change notification, so not logging warnings in race conditions is an argument for changing the log level and letting hooks be unset on the next dispatch. |
The hook is permanently removing after the warning so I don't think that reason is intended.
It is printed. |
Interesting. So maybe before the warning check whether that client exists? However, that's not amazing, because theoretically a client with name FOO could die, and another client starts later with the same name. So the most optimal approach is probably to remove the hooks once a client exists, together with other client cleanup code. But I have no opinion about the implementation details. |
The hook code is tricky because it's synchronous. The reason that hook removal is deferred to the next cycle is because it avoids reentrant issues.
It also currently logs on verbose every time a hook is processed: Line 206 in 9769f33 Is this "bogus verbose message"? If you don't like it appears in verbose, I can move it to debug or trace.
This is not a problem. Stale hooks only happen when the mpv client is destroyed. At this point the client's already made an explicit decision that it's not interested in mpv events anymore.
It's still being logged, just on a different log level. This doesn't prevent debugging in any way, and the clients shouldn't need to complicate programming and explicitly destroy all hooks when the current documented behavior works.
There is already an error-level log for full event queue: Line 744 in 42ff6f9 This PR just changes the log level, the log level can still be elevated for debugging if errors like this are observed. |
OK, so apparently So warn only if the client still exists? (and if you want, add a verbose message otherwise) FWIW, there was an additional opinion at the IRC channel that maybe we should never remove hooks (except if the client dies/no-longer-exits), but I think this can be a followup PR, because it might need some thought, and it's a behavioral change which is orthogonal to the unwanted warning after a client exits. |
|
I updated #15245 as we agreed on IRC. Also we don't understand the purpose of |
Hooks are implicitly removed if the mpv_handle it was registered with is destroyed. This is documented in libmpv/client.h and is expected behavior for hook removal, so it should not be treated as a warning. Make it verbose instead.
The log level for "client removed during hook handling" is unchanged because the documentation says it should be avoided.