-
Notifications
You must be signed in to change notification settings - Fork 21
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
Conversation
19820c5
to
cc863c2
Compare
cc863c2
to
5a04436
Compare
5a04436
to
0200db4
Compare
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) I would argue that setting a timeout for |
I see two problems there:
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 |
@l4l Quick update after more testing. Adding a breakpoint in main.rs line 264 (after the Calling |
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. |
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 |
Yes, just tested it, works fine. |
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.