fix: reduce CPU usage when stdin is null#246
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
jasom
force-pushed
the
fix/stdin-eof-polling
branch
from
3afaf6b to
7e4ae18
Compare
There are some use-cases in which stdin will not be attached to the shpool client. Right now when that happens, one thread is busy-waiting on stdin. This fix adds a 200ms delay to the polling. refs: shell-pool#243
jasom
force-pushed
the
fix/stdin-eof-polling
branch
from
7e4ae18 to
84f2433
Compare
ethanpailes
left a comment
ethanpailes
left a comment
There was a problem hiding this comment.
I'm not sure that this is the best fix for this problem. shpool attach processes should always corrispond to an actually attached terminal, so the right thing to do is probably to just detach from the session when we read an EOF off of stdin.
This is probably worth merging for now, but I think we should leave #243 open until we can write the more principled detach fix.
| loop { | ||
| let nread = stdin.read(&mut buf).context("reading stdin from user")?; | ||
| if nread == 0 { | ||
| thread::sleep(STDIN_POLL_DUR); |
There was a problem hiding this comment.
Do you mind adding a comment here explaining that this is just a tmp mitigation until we can gracefully detach in the face of EOF.
There was a problem hiding this comment.
I think hanging up here may be as simple as just returning Ok(()). It looks like that breaks a few tests, though hopefully just for timing reasons, so it's not entirely trivial. We probably want to write a new test ensuring that shpool attach with a null stdin does create the session as you would expect and the early hangup doesn't cause trouble as well.
ethanpailes
changed the title
|
I'm going to close this in favor of #306, which implements the more complete fix I was talking about. |
2 participants
There are some use-cases in which stdin will not be attached to the shpool client. Right now when that happens, one thread is busy-waiting on stdin. This fix adds a 200ms delay to the polling.
Fixes #243