-
Notifications
You must be signed in to change notification settings - Fork 3
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
don't fail if subprocess finished #166
Conversation
e.g. the user exited `less` before all results were written... example from joern _before_ this PR: ``` importCpg("/home/mp/tmp/cpgtesting/linux-kernel/linux6-drivers-scsi.fg") cpg.method.name.browse Exception in thread "less stdin thread" java.io.IOException: Broken pipe at java.base/java.io.FileOutputStream.writeBytes(Native Method) at java.base/java.io.FileOutputStream.write(FileOutputStream.java:349) at java.base/java.io.BufferedOutputStream.write(BufferedOutputStream.java:123) at java.base/sun.nio.cs.StreamEncoder.writeBytes(StreamEncoder.java:234) at java.base/sun.nio.cs.StreamEncoder.implWrite(StreamEncoder.java:304) at java.base/sun.nio.cs.StreamEncoder.implWrite(StreamEncoder.java:282) at java.base/sun.nio.cs.StreamEncoder.write(StreamEncoder.java:132) at java.base/java.io.OutputStreamWriter.write(OutputStreamWriter.java:205) at java.base/java.io.BufferedWriter.flushBuffer(BufferedWriter.java:120) at java.base/java.io.BufferedWriter.write(BufferedWriter.java:233) at replpp.Operators$$anon$1.processInput$$anonfun$1(Operators.scala:140) at java.base/java.lang.Thread.run(Thread.java:840) ``` This PR silences this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
catching Throwable
is a sharp sword - I'd prefer to see this just catching IOException
, or at least NonFatal
} | ||
} | ||
// flush stdin, but ignore errors | ||
try {stdin.flush()} catch { case t: Throwable => /*ignore*/ } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be safe to just drop this line - closing already implies pushing out all unwritten data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's what I thought too, but unfortunately that's not the case.
check out the two runs above, I only added this line after seeing the red tests, which were caused by non-flushiness
I agree re NonFatal though, just changed that part. |
e.g. the user exited
less
before all results were written...example from joern before this PR:
This PR silences this issue.