-
Notifications
You must be signed in to change notification settings - Fork 301
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
HPCC-25686 Thor crash in FileIO->close() #14844
HPCC-25686 Thor crash in FileIO->close() #14844
Conversation
https://track.hpccsystems.com/browse/HPCC-25686 |
@jakesmith do you also think we should throw if we cannot open the file ?
|
@mckellyln - it already throws an exception if fails to open (see line 802) |
@@ -433,14 +433,17 @@ void CDiskWriteSlaveActivityBase::close() | |||
outraw.clear(); | |||
} | |||
|
|||
Owned<IFileIO> tmpFileIO; | |||
if (outputIO) |
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.
looking again, if outputIO is unset, it implies neither 'out' or 'outraw' can be set either.
It won't make any practical difference, but clearer if this conditional block includes lines 417-434 too.
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.
ok, I will move it there. thx
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.
Not imperative, but one improvement suggestion.
@jakesmith pushed update. If ok I will squash |
Signed-off-by: Mark Kelly <mark.kelly@lexisnexisrisk.com>
Squashed |
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.
Looks good.
Automated Smoketest: ✅ Unit tests result:
Regression test result:
HPCC Stop: OK
|
Signed-off-by: Mark Kelly mark.kelly@lexisnexisrisk.com
Type of change:
Checklist:
Smoketest:
Testing:
smoke test