-
Notifications
You must be signed in to change notification settings - Fork 631
[CDEC-470] Catch IOExceptions in productionReporter
#3365
Conversation
ceb2e71
to
6bea9c3
Compare
died when it tried to report over the down network. (CDEC-470 / [PR 3365]) | ||
|
||
[PR 3365]: https://github.com/input-output-hk/cardano-sl/pull/3365 | ||
|
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.
👍
lib/src/Pos/Reporting/Production.hs
Outdated
@@ -31,10 +33,17 @@ productionReporter | |||
-> Reporter IO | |||
productionReporter params diffusion = Reporter $ \rt -> withWlogTempFile logConfig $ \mfp -> do | |||
rt' <- extendWithNodeInfo diffusion rt | |||
reportNode logTrace protocolMagic compileTimeInfo servers mfp rt' | |||
(reportNode logTrace protocolMagic compileTimeInfo servers mfp rt' |
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.
Are parens really needed here?
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.
I don't think so - I had them there to make the initial lack-of-indentation less confusing. But with your suggested indent of the bottom 2 lines, they seem unnecessary. Just force-pushed a fix.
6bea9c3
to
8a5922e
Compare
Reporting is currently complex and brittle, and one failure mode occurs when the network connection goes down, and reporting tries to report, but dies due to an IOException because the network is down. Installing a simple `catchIO` handler around `reportNode` should catch such exceptions and report their occurence in the log.
8a5922e
to
30f3621
Compare
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.
Great!
if this gets merged we can then adapt it to the new logging (PR #3401)
This at least seems correct. So YOLO! |
🤘 |
Description
Reporting is currently complex and brittle, and one failure mode occurs
when the network connection goes down, and reporting tries to report,
but dies due to an IOException because the network is down.
Installing a simple
catchIO
handler aroundreportNode
should catchsuch exceptions and report their occurence in the log.
Linked issue
https://iohk.myjetbrains.com/youtrack/issue/CDEC-470
Type of change
Developer checklist
Testing checklist
^ this PR modifies IO/orchestration layer code, which is not currently covered by tests.
QA Steps
Screenshots (if available)
https://asciinema.org/a/DlqCqrcv01NgUzNEiZhIKVP9w
~2:10 to ~8:30 are boring (just waiting until the worker should die, if exceptions weren't handled properly)