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

Workbook is not closed #131

Closed
alecxe opened this Issue Jun 4, 2014 · 13 comments

Comments

4 participants
@alecxe
Contributor

alecxe commented Jun 4, 2014

This is a documentation issue.

On the Working with Dates and Time page, there is an example that does not contain workbook.close() at the end. This can lead to a RuntimeError:

Exception RuntimeError: RuntimeError('sys.meta_path must be a list of import hooks',) in <bound method Workbook.__del__ of <xlsxwriter.workbook.Workbook object at 0x10617f150>> ignored

Reproduceable using XlsxWriter version 0.5.5 and Python 2.7.5. Here's the code that reproduces the problem:

from datetime import datetime
import xlsxwriter

# Create a workbook and add a worksheet.
workbook = xlsxwriter.Workbook('datetimes.xlsx')
worksheet = workbook.add_worksheet()
bold = workbook.add_format({'bold': True})

# Expand the first columns so that the dates are visible.
worksheet.set_column('A:B', 30)

# Write the column headers.
worksheet.write('A1', 'Formatted date', bold)
worksheet.write('B1', 'Format', bold)

# Create a datetime object to use in the examples.

date_time = datetime.strptime('2013-01-23 12:30:05.123',
                              '%Y-%m-%d %H:%M:%S.%f')

# Examples date and time formats.
date_formats = (
    'dd/mm/yy',
    'mm/dd/yy',
    'dd m yy',
    'd mm yy',
    'd mmm yy',
    'd mmmm yy',
    'd mmmm yyy',
    'd mmmm yyyy',
    'dd/mm/yy hh:mm',
    'dd/mm/yy hh:mm:ss',
    'dd/mm/yy hh:mm:ss.000',
    'hh:mm',
    'hh:mm:ss',
    'hh:mm:ss.000',
)

# Start from first row after headers.
row = 1

# Write the same date and time using each of the above formats.
for date_format_str in date_formats:

    # Create a format for the date or time.
    date_format = workbook.add_format({'num_format': date_format_str,
                                      'align': 'left'})

    # Write the same date using different formats.
    worksheet.write_datetime(row, 0, date_time, date_format)

    # Also write the format string for comparison.
    worksheet.write_string(row, 1, date_format_str)

    row += 1 
@alecxe

This comment has been minimized.

Show comment
Hide comment
@alecxe

alecxe Jun 4, 2014

Contributor

Note that there can be other examples in the documentation that don't contain workbook.close() at the end. So, this can be a pretty "scary" error for a new user.

Contributor

alecxe commented Jun 4, 2014

Note that there can be other examples in the documentation that don't contain workbook.close() at the end. So, this can be a pretty "scary" error for a new user.

@jmcnamara jmcnamara added bug labels Jun 5, 2014

@jmcnamara jmcnamara self-assigned this Jun 5, 2014

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jun 5, 2014

Owner

Hi Alexander,

Thanks for that. I thought that I already had an issue open to improve the docs/examples around close() or else try fix it, but clearly not.

Basically, any example that includes write_string() requires an explicit close due to a circular dependency with the SST table preventing the GC from triggering the destructors in the right sequence.

That is something that xlwt does better. By putting the filename in close() it guarantees an explicit close.

I could look at weakening the dependencies. I don't know if that would fix this issue in all cases across all Pythons.

Is that something that you would like to have a look at fixing? I'm backed up on another project at the moment.

John

Owner

jmcnamara commented Jun 5, 2014

Hi Alexander,

Thanks for that. I thought that I already had an issue open to improve the docs/examples around close() or else try fix it, but clearly not.

Basically, any example that includes write_string() requires an explicit close due to a circular dependency with the SST table preventing the GC from triggering the destructors in the right sequence.

That is something that xlwt does better. By putting the filename in close() it guarantees an explicit close.

I could look at weakening the dependencies. I don't know if that would fix this issue in all cases across all Pythons.

Is that something that you would like to have a look at fixing? I'm backed up on another project at the moment.

John

@alecxe

This comment has been minimized.

Show comment
Hide comment
@alecxe

alecxe Jun 5, 2014

Contributor

Hi John,

thanks for the quick response and the explanation.

I think there are two separate issues here: improving the docs to do the explicit close() in the "complete" snippets in the docs; and researching&fixing RuntimeError appearing if a workbook wasn't properly closed. Consider splitting the issue in 2.

I can work on both issues but not sure if I would fix the second one. Can give it a try though.

Thanks.

Alexander

Contributor

alecxe commented Jun 5, 2014

Hi John,

thanks for the quick response and the explanation.

I think there are two separate issues here: improving the docs to do the explicit close() in the "complete" snippets in the docs; and researching&fixing RuntimeError appearing if a workbook wasn't properly closed. Consider splitting the issue in 2.

I can work on both issues but not sure if I would fix the second one. Can give it a try though.

Thanks.

Alexander

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jun 5, 2014

Owner

That makes sense to split it in two.

For item 1 the docs should just state that an explicit close is always required and all examples should have it included.

For reference here are the docs for close() in the Lua version of the module.

For 2 I tried very quickly to use weakref without success. A more quick and dirty way might be to catch the exception in the workbook destructor and issue a warning. Something like the following patch:

diff --git a/xlsxwriter/workbook.py b/xlsxwriter/workbook.py
index 882f0f2..62d767b 100644
--- a/xlsxwriter/workbook.py
+++ b/xlsxwriter/workbook.py
@@ -126,8 +126,12 @@ class Workbook(xmlwriter.XMLwriter):

     def __del__(self):
         """Close file in destructor if it hasn't been closed explicitly."""
-        if not self.fileclosed:
-            self.close()
+        try:
+            if not self.fileclosed:
+                self.close()
+        except:
+            raise Exception("Exception caught in workbook destructor. "
+                            "Explicit close() may be required for workbook.")

Which would give an exception like this for the example above:

$ python date_example.py
Exception Exception: Exception('Exception caught in workbook destructor. Explicit close() may be required for workbook.',) 
in <bound method Workbook.__del__ of <xlsxwriter.workbook.Workbook object at 0x103297d50>> ignored

This was just a quick fix and you may have some better suggestions.

Note, a little unfortunately the exception isn't always RuntimeError or the try could be more explicit. On Windows and possibly some other systems/programs it raises a LookupError exception.

If you would like to tackle either of these that would be fine by me (as I said I have a bit of a backlog at the moment). If not I'll try roll them into the next release.

Owner

jmcnamara commented Jun 5, 2014

That makes sense to split it in two.

For item 1 the docs should just state that an explicit close is always required and all examples should have it included.

For reference here are the docs for close() in the Lua version of the module.

For 2 I tried very quickly to use weakref without success. A more quick and dirty way might be to catch the exception in the workbook destructor and issue a warning. Something like the following patch:

diff --git a/xlsxwriter/workbook.py b/xlsxwriter/workbook.py
index 882f0f2..62d767b 100644
--- a/xlsxwriter/workbook.py
+++ b/xlsxwriter/workbook.py
@@ -126,8 +126,12 @@ class Workbook(xmlwriter.XMLwriter):

     def __del__(self):
         """Close file in destructor if it hasn't been closed explicitly."""
-        if not self.fileclosed:
-            self.close()
+        try:
+            if not self.fileclosed:
+                self.close()
+        except:
+            raise Exception("Exception caught in workbook destructor. "
+                            "Explicit close() may be required for workbook.")

Which would give an exception like this for the example above:

$ python date_example.py
Exception Exception: Exception('Exception caught in workbook destructor. Explicit close() may be required for workbook.',) 
in <bound method Workbook.__del__ of <xlsxwriter.workbook.Workbook object at 0x103297d50>> ignored

This was just a quick fix and you may have some better suggestions.

Note, a little unfortunately the exception isn't always RuntimeError or the try could be more explicit. On Windows and possibly some other systems/programs it raises a LookupError exception.

If you would like to tackle either of these that would be fine by me (as I said I have a bit of a backlog at the moment). If not I'll try roll them into the next release.

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jun 5, 2014

Owner

Also, just to clarify.

The __del__ exception will also be triggered by any other uncaught exception in the program even if close() is included. For example here is the output for the same program with a user exception but with an implicit close:

$ python date_example.py
Traceback (most recent call last):
  File "date_example.py", line 57, in <module>
    raise Exception("XXXXXXXXXXXXXXX")
Exception: XXXXXXXXXXXXXXX
Exception Exception: Exception('Exception caught in workbook destructor. Explicit close() may be required for workbook.',) 
in <bound method Workbook.__del__ of <xlsxwriter.workbook.Workbook object at 0x1072fead0>> ignored

This might also cause confusion but probably less confusion than the current situation.

Owner

jmcnamara commented Jun 5, 2014

Also, just to clarify.

The __del__ exception will also be triggered by any other uncaught exception in the program even if close() is included. For example here is the output for the same program with a user exception but with an implicit close:

$ python date_example.py
Traceback (most recent call last):
  File "date_example.py", line 57, in <module>
    raise Exception("XXXXXXXXXXXXXXX")
Exception: XXXXXXXXXXXXXXX
Exception Exception: Exception('Exception caught in workbook destructor. Explicit close() may be required for workbook.',) 
in <bound method Workbook.__del__ of <xlsxwriter.workbook.Workbook object at 0x1072fead0>> ignored

This might also cause confusion but probably less confusion than the current situation.

@ramnes

This comment has been minimized.

Show comment
Hide comment
@ramnes

ramnes Jul 21, 2014

Just ran into this issue, just by executing the simplest possible code:

from xlsxwriter import Workbook

w = Workbook("test.xlsx")
# EOF

It's bit disturbing and as a developer using a library, you don't except that kind of error with such an unrelated exception message.

ramnes commented Jul 21, 2014

Just ran into this issue, just by executing the simplest possible code:

from xlsxwriter import Workbook

w = Workbook("test.xlsx")
# EOF

It's bit disturbing and as a developer using a library, you don't except that kind of error with such an unrelated exception message.

jmcnamara added a commit that referenced this issue Jul 21, 2014

Made close() a required method call.
Updated the documentation and desctructor exception handler to make
it clear that an implicit close() is required. This is to avoid
confusing errors when an implicit close isn't called by the GC.
Fix for issue #131.
@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jul 22, 2014

Owner

Fixed in version 0.5.6. Thanks.

John

Owner

jmcnamara commented Jul 22, 2014

Fixed in version 0.5.6. Thanks.

John

@jmcnamara jmcnamara closed this Jul 22, 2014

@ramnes

This comment has been minimized.

Show comment
Hide comment
@ramnes

ramnes Jul 22, 2014

I did not look the code deep, but it seems to me that a better approach would be to handle the garbage collector calling Workbook.__del__, and patch whatever is raising this RuntimeError instead of wraping it in a kind of informative exception.

ramnes commented Jul 22, 2014

I did not look the code deep, but it seems to me that a better approach would be to handle the garbage collector calling Workbook.__del__, and patch whatever is raising this RuntimeError instead of wraping it in a kind of informative exception.

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jul 22, 2014

Owner

Hi Guillaume,

it seems to me that a better approach would be to handle the garbage collector calling Workbook.del, and patch whatever is raising this RuntimeError

That seems to be easier said then done.

There isn't one single exception that is raised. However, they are all related to worksheets being garbage collected before the workbook is finished using them. I tried to weaken the circular dependencies with weakref but that didn't work (read back through the comments above for context).

If you have some idea about how to resolve it in a more robust manner, let me know.

John

Owner

jmcnamara commented Jul 22, 2014

Hi Guillaume,

it seems to me that a better approach would be to handle the garbage collector calling Workbook.del, and patch whatever is raising this RuntimeError

That seems to be easier said then done.

There isn't one single exception that is raised. However, they are all related to worksheets being garbage collected before the workbook is finished using them. I tried to weaken the circular dependencies with weakref but that didn't work (read back through the comments above for context).

If you have some idea about how to resolve it in a more robust manner, let me know.

John

@ramnes

This comment has been minimized.

Show comment
Hide comment
@ramnes

ramnes Jul 22, 2014

I understand. My point is just to say that this issue should be left open, because it's not really resolved. Or maybe you could sum up this circular dependencies problem in a new issue?
I'll try to take a look at this, anyway.

ramnes commented Jul 22, 2014

I understand. My point is just to say that this issue should be left open, because it's not really resolved. Or maybe you could sum up this circular dependencies problem in a new issue?
I'll try to take a look at this, anyway.

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jul 22, 2014

Owner

My point is just to say that this issue should be left open, because it's not really resolved.

The issue as reported is resolved. As is the secondary issue of semi-random exception messages.

The underlying issue of circular dependencies isn't resolved but, I agree, that should be a separate issue. If you would like to open an issue to submit a PR against then please do. Otherwise I wouldn't be in favour of opening an issue unless it directly affects someone since there is no guarantee that it can be fixed.

I'll try to take a look at this, anyway.

Great. Thanks.

Owner

jmcnamara commented Jul 22, 2014

My point is just to say that this issue should be left open, because it's not really resolved.

The issue as reported is resolved. As is the secondary issue of semi-random exception messages.

The underlying issue of circular dependencies isn't resolved but, I agree, that should be a separate issue. If you would like to open an issue to submit a PR against then please do. Otherwise I wouldn't be in favour of opening an issue unless it directly affects someone since there is no guarantee that it can be fixed.

I'll try to take a look at this, anyway.

Great. Thanks.

@jkyeung

This comment has been minimized.

Show comment
Hide comment
@jkyeung

jkyeung Jul 23, 2014

Contributor

I don't think it's worth reopening this or submitting a new doc bug, but right now the docs for workbook.close() state:

An implicit close() is now recommended in all XlsxWriter programs.

That should be explicit.

Contributor

jkyeung commented Jul 23, 2014

I don't think it's worth reopening this or submitting a new doc bug, but right now the docs for workbook.close() state:

An implicit close() is now recommended in all XlsxWriter programs.

That should be explicit.

@jmcnamara

This comment has been minimized.

Show comment
Hide comment
@jmcnamara

jmcnamara Jul 23, 2014

Owner

@jkyeung

That should be explicit.

Thanks John. Well spotted. That is fixed now.

Owner

jmcnamara commented Jul 23, 2014

@jkyeung

That should be explicit.

Thanks John. Well spotted. That is fixed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment