player/client: reduce log level for hooks not sent to clients #15245
Conversation
guidocella
changed the title
|
Download the artifacts for this pull request: |
guidocella
force-pushed
the
auto-profiles-hook
branch
from
72f826c
to
446825f
Compare
guidocella
force-pushed
the
auto-profiles-hook
branch
from
446825f
to
83dc82a
Compare
player/command.c
Outdated
| mp_wakeup_core(mpctx); // repeat next iteration to finish | ||
| if (mp_client_id_exists(mpctx, h->client_id)) { | ||
| MP_VERBOSE(mpctx, "Sending hook command failed. Removing hook.\n"); | ||
| hook_remove(mpctx, h); |
There was a problem hiding this comment.
Why is mp_wakeup_core removed?
There was a problem hiding this comment.
It was in the wrong block, I just moved it in below, I have no idea if it needs to run in both cases.
player/command.c
Outdated
| MP_VERBOSE(mpctx, "Sending hook command failed. Removing hook.\n"); | ||
| hook_remove(mpctx, h); | ||
| } else { | ||
| MP_WARN(mpctx, "Sending hook command failed.\n"); |
There was a problem hiding this comment.
I think the hook removal should be kept, because it results in behavior change if the hook isn't removed. This would break clients which unknowingly assumes that they can take longer time to process the hooks and not blocking the player because the player will eventually unblock itself due to hook removal.
The same is also done on other software, for example Windows which silently removes low level hooks if the clients don't respond in time.
There was a problem hiding this comment.
We can indeed remove hooks, I think it is clients fault anyway. But could we document this behavior? That hooks are removed on any and all errors?
Also it is kinda tricky, because client doesn't have a way to know if the hooks has been removed, so it might left script in unusable state, as the hooks are not one-shoot events.
There was a problem hiding this comment.
I changed only the log level now.
There was a problem hiding this comment.
Also it is kinda tricky, because client doesn't have a way to know if the hooks has been removed, so it might left script in unusable state,
Funnily, this is also exactly what happens when windows stops sending hooks to apps it thinks misbehave. So mpv is not the first to do that kind of thing.
I think both approaches are valid, but hooks specifically are synchronous and would definitely hurt mpv if not handled on time, so there's a bigger incentive here to break abusing clients.
guidocella
force-pushed
the
auto-profiles-hook
branch
2 times, most recently
from
b9d2993
to
1f91242
Compare
|
Commit "player/client: reduce log level for hooks not sent to clients" LGTM. No comment on the autoprofiles commit. |
|
The autoprofile commit shouldn't be needed anymore since script exiting removing hooks is expected behavior, and there are no longer logs at the default level. Scripts shouldn't need to do anything special about the hooks if they want to |
|
"Fixes #15244" should probably be moved to the C commit.
I prefer to stay out of autoprofiles, but I think the same. |
guidocella
force-pushed
the
auto-profiles-hook
branch
from
1f91242
to
f154598
Compare
guidocella
changed the title
guidocella
force-pushed
the
auto-profiles-hook
branch
from
f154598
to
902edd7
Compare
If a hook event can't be sent to a client because it no longer exists, stop logging it as a warning, as there is no way for a client to remove hooks, so it is expected that the hook can't be sent. This is documented in libmpv/client.h. If the hook event can't be sent for other reasons, like the event queue being full (currently the only other possible reason), keep logging as warning. Also add the client and hook type to the message. They are also logged just above, but only in verbose mode, so when only the warning is logged you didn't see the client before. auto_profiles.lua logs these warning since 5dc4047, and this commit fixes that. Fixes mpv-player#15244.
guidocella
force-pushed
the
auto-profiles-hook
branch
from
902edd7
to
0b90594
Compare
|
Thanks for adding the client/hook names to the message. This can be helpful. The message format change is also OK (I would have only added LGTM. I'll merge it a bit later unless there are further comments. |
If a script registers hooks before exiting mpv logs "Sending hook command failed. Removing hook.". (Also there is no function to unregister hooks.) Don't register hooks in auto_profiles.lua before it exits to not log these warnings. Fixes 5dc4047. Fixes #15244.