Handle Open3 returning a nil status #93
Conversation
| yield out | ||
| else | ||
| raise ::CC::Engine::Analyzers::ParserError, "`#{command}` exited with code #{status.exitstatus}:\n#{err}" | ||
| exitstatus = status && status.exitstatus | ||
| raise ::CC::Engine::Analyzers::ParserError, "`#{command}` exited with code #{exitstatus}:\n#{err}" |
There was a problem hiding this comment.
Should this be exitstatus.inspect so that we get the nil explicitly printed?
|
FWIW I looked into this a bit a while back & from my notes, I believed this wasn't actually a failure case, but a race condition: the My thinking was we should move away from |
|
I'll happily move to |
pbrisbin
force-pushed
the
pb-nil-status
branch
from
3765e10 to
cdc50e0
Compare
|
Changed, squashed, and commit message updated. Ready for re-review. |
|
LGTM. |
pbrisbin
force-pushed
the
pb-nil-status
branch
2 times, most recently
from
3765e10 to
ca5d187
Compare
|
Had to go back to the original implementation because posix-spawn's not supported on JRuby. Please see the last commit for a detailed message: ca5d187 |
|
I'm cool with it. |
capture3 on JRuby will assign a pid variable as the result of spawn[1]. It then passes that to Process.detach[2]. If the process in question has exited between these two statements, Process.detach will return nil. It can't be determined then if the process had succeeded or not. Rather than always assuming an error, we use a heuristic: if the output produced parses as valid JSON, we should consider it successful. [1]: https://github.com/jruby/jruby/blob/master/lib/ruby/stdlib/open3.rb#L200 [2]: https://github.com/jruby/jruby/blob/master/lib/ruby/stdlib/open3.rb#L201
pbrisbin
force-pushed
the
pb-nil-status
branch
from
ca5d187 to
8c9b4e9
Compare
Handle Open3 returning a nil status
Customers are experiencing this in production. It results in an engine error
output of:
The included spec reproduces this error.
It's unclear what circumstances lead to this, but removing the NoMethodError
will at least let the engine's stderr come through, which we can then debug and
address thereafter.
/cc @codeclimate/review
I also added basic spec coverage before beginning, as its own commit.