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

add timeout to panic notification #149

Closed
wants to merge 2 commits into from

Conversation

BlockListed
Copy link
Contributor

@BlockListed BlockListed commented Oct 8, 2023

This fixes #148

On panic we send a notification, but if not notification daemon is available we block on notify-send for a while, which leads to issues.

@BlockListed BlockListed changed the title fix panic when no entries match query #148 fix panic when no entries match query Oct 8, 2023
@BlockListed BlockListed changed the title fix panic when no entries match query add timeout to panic notification Oct 8, 2023
@l4l
Copy link
Owner

l4l commented Oct 9, 2023

Instead of just blindly setting a timeout for child, I'd rather prefer figure out why is this happening. This behavior doesn't look rather obvious but anyway, it's better to give an option to explicitly disable the notify-send instead of silently hiding an issue.

@BlockListed
Copy link
Contributor Author

Instead of just blindly setting a timeout for child, I'd rather prefer figure out why is this happening. This behavior doesn't look rather obvious but anyway, it's better to give an option to explicitly disable the notify-send instead of silently hiding an issue.

First it doesn't silently hide the issue (we log it)
Second the lack of a timeout causes large issues, because the yofi ui is no longer updated after panicking, but it still grabs keyboard / mouse focus and therefor makes the wm fully unusable (eg. to force quit yofi).

I would argue that setting a timeout for notify-send is a correct solution, because the aforementioned keyboard / mouse focus locking during the panic, leads us to soft-locking the wm (only fixable by wm restart or switch to tty) based on the behaviour of another program.

@l4l
Copy link
Owner

l4l commented Oct 10, 2023

I see two problems there:

  • for some reason sctk destructor doesn't free libinput stuff, though it probably should;
  • notify-send shoudln't block, even without notification daemon.

The timeout for killing notify-send looks like a workaround, not a proper solution.

@BlockListed
Copy link
Contributor Author

I see two problems there:

* for some reason sctk destructor doesn't free libinput stuff, though it probably should;

* notify-send shoudln't block, even without notification daemon.

The timeout for killing notify-send looks like a workaround, not a proper solution.

Well ´notify-send´ does seem to have a timeout for around 1 minute, but I think it is completely unacceptable to lock the wm for that long. Another option could be simply spawning the notify-send process and then exiting. But fixing the input locking is probably the most correct solution.

@BlockListed
Copy link
Contributor Author

@l4l Quick update after more testing. Adding a breakpoint in main.rs line 264 (after the main_inner function) already results in a soft-lock of the WM. Maybe this should be pushed to a seperate PR. The RenderSurface drop function (and therefore the destroy function of the surfaces) definitely get called however.

Calling .set_keyboard_reactivity(KeyboardInteractivity::None) in the RenderSurface drop function also doesn't seem to fix anything (keyboard input to other programs is still not possible).

@BlockListed
Copy link
Contributor Author

BlockListed commented Oct 12, 2023

@l4l Quick update after more testing. Adding a breakpoint in main.rs line 264 (after the main_inner function) already results in a soft-lock of the WM. Maybe this should be pushed to a seperate PR. The RenderSurface drop function (and therefore the destroy function of the surfaces) definitely get called however.

Calling .set_keyboard_reactivity(KeyboardInteractivity::None) in the RenderSurface drop function also doesn't seem to fix anything (keyboard input to other programs is still not possible).

I don't really have the desire to fix this bug right now, so I put the destructor issues in #150.

I believe, that for now we should merge the shorter timeout, especially since notify-send doesn't seem to have any native timeout functionality.

@l4l
Copy link
Owner

l4l commented Oct 18, 2023

Removing notification daemon doesn't reproduce, but I finally make it work while stopping the real daemon. I think I'll rewrite the notify-send or just use a library eventually but for now I'd rather prefer a simpler version with timeout binary: #151. Could you check it works for you?

@l4l l4l mentioned this pull request Oct 18, 2023
@BlockListed
Copy link
Contributor Author

Removing notification daemon doesn't reproduce, but I finally make it work while stopping the real daemon. I think I'll rewrite the notify-send or just use a library eventually but for now I'd rather prefer a simpler version with timeout binary: #151. Could you check it works for you?

Yes, just tested it, works fine.

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.

Panic when no apps match query
2 participants