fix: replace try! with try? for stdout/stderr writes in ProcessIO - #1784
Conversation
jglogan
left a comment
jglogan
left a comment
There was a problem hiding this comment.
@SEPURI-SAI-KRISHNA Thank you for the contribution! Please have a look at the comments and let me know what you think.
| return | ||
| } | ||
| try! pout.write(contentsOf: data) | ||
| try? pout.write(contentsOf: data) |
There was a problem hiding this comment.
Thanks for catching these force-try cases.
I think we'd be better served in this case if we treat these failed writse like an EOF:
| try? pout.write(contentsOf: data) | |
| do { | |
| try pout.write(contentsOf: data) | |
| } catch { | |
| rout.readabilityHandler = nil | |
| cc.yield() | |
| } |
| return | ||
| } | ||
| try! perr.write(contentsOf: data) | ||
| try? perr.write(contentsOf: data) |
There was a problem hiding this comment.
| try? perr.write(contentsOf: data) | |
| do { | |
| try perr.write(contentsOf: data) | |
| } catch { | |
| rerr.readabilityHandler = nil | |
| cc.yield() | |
| } |
a1f02be to
1c833f3
Compare
|
@jglogan try? silently drops the error, but treating a failed write as EOF is the right behavior (stop reading, signal completion). Let me apply it. |
1c833f3 to
bc4c9e7
Compare
|
@jglogan please check now, i have made the changes. |
Code Coverage
|
jglogan
left a comment
jglogan
left a comment
There was a problem hiding this comment.
@SEPURI-SAI-KRISHNA Thank you for the fix! It's nice to chip away at this technical debt.
And thanks for submitting these as individual PRs - it's easier to review and approve them individually (I'm still thinking about the app-root one, so having it as a separate change means it doesn't hold up these).
|
@jglogan Appreciate your support and patience. Thanks for sharing your thoughts on separate PR. I follow this across GIT which I consider a good practice rather than bulky PR with huge code changes. Would definitely love to contribute further. |
…ple#1784) - In `ProcessIO.swift`, the readability handlers for stdout and stderr used `try!` when writing data to the output file handles. This would cause crashes If the pipe is broken such that the force-try executes. - Changed to handle a failed write similarly to an EOF.
3 participants
Type of Change
Motivation and Context
In
ProcessIO.swift, the readability handlers for stdout and stderr usedtry!when writing data to the output file handles. If the pipe isbroken (e.g. the terminal is closed mid-execution), this crashes the
entire process instead of gracefully continuing.
Changed both occurrences to
try?so a failed write is silently droppedthe process and its exit code are still correctly handled.
Testing