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

slumber process hangs at 100% CPU if parent process is killed #136

Closed
LucasPickering opened this issue Mar 25, 2024 · 1 comment · Fixed by #187
Closed

slumber process hangs at 100% CPU if parent process is killed #136

LucasPickering opened this issue Mar 25, 2024 · 1 comment · Fixed by #187
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@LucasPickering
Copy link
Owner

LucasPickering commented Mar 25, 2024

If slumber's parent process is killed (with SIGTERM, although not sure it matters how), instead of terminating the slumber process hangs at 100% CPU and stops responding to SIGTERM itself. I've noticed this with both watchexec and zellij as the parent.

@LucasPickering LucasPickering added the bug Something isn't working label Mar 25, 2024
@LucasPickering LucasPickering changed the title slumber process hangs at 100% CPU slumber process hangs at 100% CPU if parent process is killed Mar 25, 2024
@LucasPickering
Copy link
Owner Author

I got pretty far on diagnosing this but couldn't figure out a fix. A few things I found out:

  • Upgrading watchexec to 1.25.1 fixed the issue with watchexec (thanks to this PR)
  • Zellij sends SIGHUP to its child process on quit, and that signal isn't being handled properly
  • When the CPU goes to 100%, it's because it's stuck calling crossterm::poll
    • Despite giving a timeout, it never times out and just blocks on a read syscall
  • The issue is related to some interaction between crossterm::poll and having signal_hook catch SIGHUP
    • If you remove SIGHUP from this list, or remove the poll call, the issue disappears and the process exits gracefully

I haven't figured out a way to reproduce this outside of zellij, so it's hard to submit a bug report anywhere for it. I'm not really sure if it's a bug in crossterm or signal_hook (or neither?).

@LucasPickering LucasPickering added the good first issue Good for newcomers label Apr 7, 2024
@LucasPickering LucasPickering added help wanted Extra attention is needed and removed good first issue Good for newcomers labels Apr 18, 2024
LucasPickering added a commit that referenced this issue Apr 18, 2024
This is leftover from the unsuccessful/incomplete debugging of #136
LucasPickering added a commit that referenced this issue Apr 18, 2024
This is leftover from the unsuccessful/incomplete debugging of #136
LucasPickering added a commit that referenced this issue Apr 29, 2024
Previously, the main loop was rate-limited by the terminal input poll() call. This means when no user input was present, it would block for 250ms before handling any async messages. This could cause some annoying UI delays for changes triggered by background IO (e.g. HTTP responses could take up to 250ms *extra* to appear). By breaking the input loop into a separate task, and having the main loop block directly on the message queue, we ensure that every message is handled immediately and we update the UI immediately.

I measured CPU usage on this, and it's the same as master. Locally I see ~5% usage whlie running in debug and ~0.7% while running with --release. This makes sense because the CPU is doing all the same work, just rearranged slightly.

This has the added benefit that it accidentally fixed #136, which is great because I really didn't want to debug that anymore.
LucasPickering added a commit that referenced this issue Apr 29, 2024
Previously, the main loop was rate-limited by the terminal input poll() call. This means when no user input was present, it would block for 250ms before handling any async messages. This could cause some annoying UI delays for changes triggered by background IO (e.g. HTTP responses could take up to 250ms *extra* to appear). By breaking the input loop into a separate task, and having the main loop block directly on the message queue, we ensure that every message is handled immediately and we update the UI immediately.

I measured CPU usage on this, and it's the same as master. Locally I see ~5% usage whlie running in debug and ~0.7% while running with --release. This makes sense because the CPU is doing all the same work, just rearranged slightly.

This has the added benefit that it accidentally fixed #136, which is great because I really didn't want to debug that anymore.
LucasPickering added a commit that referenced this issue Apr 29, 2024
Previously, the main loop was rate-limited by the terminal input poll() call. This means when no user input was present, it would block for 250ms before handling any async messages. This could cause some annoying UI delays for changes triggered by background IO (e.g. HTTP responses could take up to 250ms *extra* to appear). By breaking the input loop into a separate task, and having the main loop block directly on the message queue, we ensure that every message is handled immediately and we update the UI immediately.

I measured CPU usage on this, and it's the same as master. Locally I see ~5% usage whlie running in debug and ~0.7% while running with --release. This makes sense because the CPU is doing all the same work, just rearranged slightly.

This has the added benefit that it accidentally fixed #136, which is great because I really didn't want to debug that anymore.
LucasPickering added a commit that referenced this issue Apr 29, 2024
Previously, the main loop was rate-limited by the terminal input poll() call. This means when no user input was present, it would block for 250ms before handling any async messages. This could cause some annoying UI delays for changes triggered by background IO (e.g. HTTP responses could take up to 250ms *extra* to appear). By breaking the input loop into a separate task, and having the main loop block directly on the message queue, we ensure that every message is handled immediately and we update the UI immediately.

I measured CPU usage on this, and it's the same as master. Locally I see ~5% usage whlie running in debug and ~0.7% while running with --release. This makes sense because the CPU is doing all the same work, just rearranged slightly.

This has the added benefit that it accidentally fixed #136, which is great because I really didn't want to debug that anymore.
LucasPickering added a commit that referenced this issue Apr 29, 2024
Previously, the main loop was rate-limited by the terminal input poll() call. This means when no user input was present, it would block for 250ms before handling any async messages. This could cause some annoying UI delays for changes triggered by background IO (e.g. HTTP responses could take up to 250ms *extra* to appear). By breaking the input loop into a separate task, and having the main loop block directly on the message queue, we ensure that every message is handled immediately and we update the UI immediately.

I measured CPU usage on this, and it's the same as master. Locally I see ~5% usage whlie running in debug and ~0.7% while running with --release. This makes sense because the CPU is doing all the same work, just rearranged slightly.

This has the added benefit that it accidentally fixed #136, which is great because I really didn't want to debug that anymore.
LucasPickering added a commit that referenced this issue Apr 29, 2024
Previously, the main loop was rate-limited by the terminal input poll() call. This means when no user input was present, it would block for 250ms before handling any async messages. This could cause some annoying UI delays for changes triggered by background IO (e.g. HTTP responses could take up to 250ms *extra* to appear). By breaking the input loop into a separate task, and having the main loop block directly on the message queue, we ensure that every message is handled immediately and we update the UI immediately.

I measured CPU usage on this, and it's the same as master. Locally I see ~5% usage whlie running in debug and ~0.7% while running with --release. This makes sense because the CPU is doing all the same work, just rearranged slightly.

This has the added benefit that it accidentally fixed #136, which is great because I really didn't want to debug that anymore.
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue May 6, 2024
[1.1.0] - 2024-05-05

Added
- Add `section` field to `!request` chain values, to allow chaining response headers rather than body ([#184](LucasPickering/slumber#184))
- Add action to save response body to file ([#183](LucasPickering/slumber#183))
- Add `theme` field to the config, to configure colors ([#193](LucasPickering/slumber#193))
  - [See docs](https://slumber.lucaspickering.me/book/api/configuration/theme.html) for more info
- Add `stdin` option to command chains ([#190](LucasPickering/slumber#190))

Changed
- Reduce UI latency under certain scenarios
  - Previously some actions would feel laggy because of an inherent 250ms delay in processing some events
- Search parent directories for collection file ([#194](LucasPickering/slumber#194))
- Use thicker borders for selected pane and modals
- Change default TUI colors to blue and yellow

Fixed
- Fix Slumber going into zombie mode and CPU spiking to 100% under certain closure scenarios ([#136](LucasPickering/slumber#136))
- Fix historical request/response no loading on first render ([#199](LucasPickering/slumber#199))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant