fix(table): close writers on error for every exit path #667
Conversation
ferhatelmas
force-pushed
the
fix-writers
branch
from
ea38d98 to
7dbc458
Compare
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer internal.CheckedClose(wr, &err) |
There was a problem hiding this comment.
this doesn't cause an error when we successfully call close at line 300?
There was a problem hiding this comment.
Lines 1124 to 1126 in 9e4a5b7
no because it's no-op, Close() sets closed field to true (first defer) and when it's true (second defer), it returns early
There was a problem hiding this comment.
Sounds good then. Is there anything else needed before you shift this out of Draft?
There was a problem hiding this comment.
I reviewed for the pattern if there were more and found a few more cases. Fixed and added a regression test, should be good to go now. Marking ready
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
just one nitpick question but otherwise looks good!
ferhatelmas
force-pushed
the
fix-writers
branch
from
7dbc458 to
c466baa
Compare
Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
ferhatelmas
force-pushed
the
fix-writers
branch
from
c466baa to
66a09f7
Compare
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM! I double checked for all usage of NewManifestWriter and ManifestListWriter
3 participants
related to #644