improve io waiting, ensure timeout thread runs immediately #225
Conversation
| @@ -60,7 +60,7 @@ def run(options = []) | |||
| duration = timeout | |||
| @listener.timed_out(container_data(duration: duration)) | |||
| else | |||
| sleep 0.01 until !t_out.alive? && !t_err.alive? | |||
| t_out.join || t_err.join | |||
There was a problem hiding this comment.
Is it possible for one of these to finish before the other? Imagining if stderr closes but stdout is still flushing? Is that possible?
There was a problem hiding this comment.
I think it'd be safest / simplest to just join both
There was a problem hiding this comment.
Yep, you're right. That was my intent but I messed up my boolean logic.
wfleming
force-pushed
the
will/timeout
branch
from
e7b19bc
to
4321c70
Compare
This is an optimization of the fix from 320eda4. The previous code in a tight loop using `sleep` seemed like it thrashed CPU context switches. Hence why it could potentially take quite a lot longer, as evidenced by the spec change. This should have the same behavior: `Thread#value` is a blocking call that waits until the thread has finished. But this avoids the `sleep` on the main thread which resulted in thread thrashing. I have also devised a spec that actually tests this behavior reasonably well. It's a bit hacky in some ways.
Just creating the thread doesn't ensure it gets run immediately. This schedules the thread immediately, so timeouts should be more prompt
|
This appears to now just remove the |
|
@pbrisbin Gordon just reverted to the |
| @@ -75,13 +75,8 @@ def run(options = []) | |||
| ) | |||
| ensure | |||
| t_timeout.kill if t_timeout | |||
There was a problem hiding this comment.
💄, but looking at this now I think it would be nicer to read as [t_timeout, t_out, t_err].each { |t| t.kill if t }
There was a problem hiding this comment.
I think doing that would be an issue if something raised an error before one of them was initialized (which is the purpose of the if t_out right?)
There was a problem hiding this comment.
Ah, good point, but, that being the case, I'm not sure the existing code is correct either: t_out.kill if t_out will also raise an exception if t_out never got defined. We'd want to either use defined?(t_tout), or switch to instance vars that have the nil-if-undefined behavior.
There was a problem hiding this comment.
Hmm that might be right. I'll use the array and see what happens
| @timed_out = true | ||
| reap_running_container | ||
| end | ||
| end.run |
There was a problem hiding this comment.
Curious why this call was absent before but present now?
There was a problem hiding this comment.
This is from @wfleming's original change - it's explained in the PR description
In testing, I was seeing the `sleep timeout` be skipped. Doing multiple smaller sleeps seems to be more reliable.
|
Would like support to mercy merge this as it could help fix the waiting status |
| @listener.timed_out(container_data(duration: duration)) | ||
| else | ||
| sleep 0.01 until !t_out.alive? && !t_err.alive? | ||
| sleep 1 until !t_out.alive? && !t_err.alive? |
There was a problem hiding this comment.
Comment lost to the diff:
I wondered this last time it came though: doesn't this basically just re-implement Thread#join?
There was a problem hiding this comment.
@wfleming had a reason to back that change out - will, what was that?
improve io waiting, ensure timeout thread runs immediately
This is working for me locally, and I have managed to make things timeout correctly locally. It should also be noted that timeout behavior was already specced as well, and those specs were running fine. So I think the missed timeout I saw in production this morning was something different, possibly an isolated problem, but almost certainly not a consequence of the other recent changes. This should tighten things up overall. I'll continue to dig into potential ways timeouts could be delayed or missed as well.
👓 @codeclimate/review