Skip to content

ARROW-1768: [Python] Fix suppressed exception in ParquetWriter.__del__#1286

Closed
Licht-T wants to merge 2 commits intoapache:masterfrom
Licht-T:fix-suppressed-exception-in-parquetwriter-del
Closed

ARROW-1768: [Python] Fix suppressed exception in ParquetWriter.__del__#1286
Licht-T wants to merge 2 commits intoapache:masterfrom
Licht-T:fix-suppressed-exception-in-parquetwriter-del

Conversation

@Licht-T
Copy link
Contributor

@Licht-T Licht-T commented Nov 5, 2017

This closes ARROW-1768.

{0}
""".format(_parquet_writer_arg_docs)

is_open = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the correct solution. It looks like we're defining __del__ which will be called if __init__ raises an exception. If __init__ raises an exception before defining self.is_open then you'll get an AttributeError exception in __del__ when it's called.

We really should not be opening files in constructors, since Python doesn't have the same guarantees as C++ around resource initialization and destruction. We should be using a context manager here since that provides clear semantics and guarantees when __enter__ and __exit__ will run.

For now, though, can you do this instead?

def __del__(self):
    if getattr(self, 'is_open', False):
        self.close()

Change-Id: I08f5e533205214de74f340dec2cfd84931782ad0
Copy link
Contributor

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wesm wesm closed this in 3995eb3 Nov 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants