Title: Files are not flushed properly when opened from the EDT
Type: Severity: urgent
Components: Core Versions: Jython 2.5
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: pjenvey Nosy List: alex.gronholm, fwierzbicki, pjenvey, zyasoft
Priority: urgent Keywords: patch

Created on 2011-01-25.17:35:06 by alex.gronholm, last changed 2014-06-17.22:46:20 by zyasoft.

File name Uploaded Description Edit Remove alex.gronholm, 2011-01-25.17:35:05
issue1701-part1.diff pjenvey, 2011-02-08.01:30:49
msg6353 (view) Author: Alex Grönholm (alex.gronholm) Date: 2011-01-25.17:35:05
If a file is opened and written to from the Event Dispatch Thread, the contents do not get flushed when the file is closed.

Attached is a simple Swing application that writes the TEST file when the button is pushed. It also writes a TEST2 file for comparison, but in the main thread. On Jython 2.5.2b2 and higher, TEST ends up as a 0 byte file, while in 2.5.2b1 and lower it ends up the same size as TEST2.
msg6362 (view) Author: Philip Jenvey (pjenvey) Date: 2011-01-30.21:17:40
I think this was broken by the somewhat recent changes for #1327.

Here the main thread exits before the EDT writes the file. When exiting it assumes jython has finished and calls interp.cleanup() to shut itself down.

So the PyFile.Closer created for the file opened in the EDT is registered with a PySystemState that has already had cleanup() called on. When this happens Closers end up not being called.
msg6363 (view) Author: Alex Grönholm (alex.gronholm) Date: 2011-01-30.21:31:05
This does not happen when writing to files from threads spawned from Jython. What makes the EDT special here?
msg6364 (view) Author: Philip Jenvey (pjenvey) Date: 2011-01-30.21:32:56
But there's probably actually 2 issues here:

1) Explicitly closing a file in this situation doesn't work. The EDT file writing in this example is using the with statement and even that doesn't flush the file. This is probably due to a little logic bug with the recent closer changes

2) However even with a quick fix for #1 I think we're still going to have the same problem when you *don't* explicitly call close on the file. 2.5.1 would flush that file on exit too

I think we can probably fix #2 for command line jython by joining all non daemon threads before having the main thread call cleanup().

Command line CPython does this. It calls threading._shutdown (which joins all threads registered with the threading module) before exiting.

Jython doesn't call threading._shutdown, but it wouldn't help this bug if it did: because the EDT thread is never registered in the threading module (threads created from pure Java are only registered with it lazily when the threading module is first used)
msg6365 (view) Author: Philip Jenvey (pjenvey) Date: 2011-01-30.21:38:12
Alex, did you use the threading module to create those threads?

I'm about 95% sure that would work in that situation because when you first import our threading module it creates a MainThread object that registers a shutdown function with atexit. So the threads you'd create would be registered with threading, and join'd during cleanup()
msg6366 (view) Author: Philip Jenvey (pjenvey) Date: 2011-01-30.21:39:59
I think joining all these threads before cleanup() would be good for the command line runner but I have to wonder if the change to Closer behavior will break anyone else's use of the PythonInterpreter API?
msg6367 (view) Author: Alex Grönholm (alex.gronholm) Date: 2011-01-30.21:40:18
Yeah, I did.
msg6368 (view) Author: Alex Grönholm (alex.gronholm) Date: 2011-01-30.21:54:15
Are you said we should be catering to those who rely on broken behavior?
msg6369 (view) Author: Philip Jenvey (pjenvey) Date: 2011-01-30.22:03:50
What broken behavior do you mean?
msg6371 (view) Author: Alex Grönholm (alex.gronholm) Date: 2011-01-31.00:21:47
"I have to wonder if the change to Closer behavior will break anyone else's use of the PythonInterpreter API?"

How should I have interpreted this then?
msg6372 (view) Author: Philip Jenvey (pjenvey) Date: 2011-01-31.00:36:20
Wasn't sure if you meant the API use or case #2 above (even if command line users don't call close() they should expect files to be closed at some point before exit).

What I was getting at is yes we should cater to the broken API usage but only to a certain extent. The original change to cleanup() is a good one but we could at least give a warning about the change in the release notes.

It'd probably be unlikely that anyone runs into problems due this change but if they do they're going to have a very mysterious problem to diagnose
msg6386 (view) Author: Philip Jenvey (pjenvey) Date: 2011-02-08.01:30:49
I propose this small patch to fix the issue #1 that I described above. AFAICT the only time sys.unregisterClose would return false would be in a situation like this. Thus it should be ok to always close the file here
msg6395 (view) Author: Philip Jenvey (pjenvey) Date: 2011-02-12.01:51:10
the small patch was applied in r7197
msg7376 (view) Author: Frank Wierzbicki (fwierzbicki) Date: 2012-08-10.20:51:33
Philip is this closable now?
msg7379 (view) Author: Philip Jenvey (pjenvey) Date: 2012-08-10.21:06:51
Unfortunately no, this bug has another outstanding issue that is not fixed yet
msg8519 (view) Author: Jim Baker (zyasoft) Date: 2014-05-22.00:26:48
This is a fairly complex issue (!), but returning to the original test Alex submitted, it now works on 2.7 trunk. TEST and TEST2 both have hello written after one clicks on the button.

So I'm putting this in pending fixed, unless some of our posters have something to add.
Date User Action Args
2014-06-17 22:46:20zyasoftsetstatus: pending -> closed
2014-05-22 00:26:49zyasoftsetstatus: open -> pending
resolution: remind -> fixed
messages: + msg8519
2013-02-20 00:06:30fwierzbickisetresolution: remind
versions: + Jython 2.5, - 2.5.2rc
2012-08-10 21:06:51pjenveysetmessages: + msg7379
2012-08-10 20:51:33fwierzbickisetmessages: + msg7376
2011-02-12 01:51:10pjenveysetmessages: + msg6395
2011-02-11 20:58:38pjenveysetassignee: pjenvey
2011-02-08 01:30:49pjenveysetfiles: + issue1701-part1.diff
keywords: + patch
messages: + msg6386
2011-01-31 00:36:21pjenveysetmessages: + msg6372
2011-01-31 00:21:47alex.gronholmsetmessages: + msg6371
2011-01-30 22:03:50pjenveysetmessages: + msg6369
2011-01-30 21:54:15alex.gronholmsetmessages: + msg6368
2011-01-30 21:40:18alex.gronholmsetmessages: + msg6367
2011-01-30 21:39:59pjenveysetmessages: + msg6366
2011-01-30 21:38:12pjenveysetmessages: + msg6365
2011-01-30 21:32:56pjenveysetmessages: + msg6364
2011-01-30 21:31:06alex.gronholmsetmessages: + msg6363
2011-01-30 21:17:41pjenveysetnosy: + pjenvey, zyasoft
messages: + msg6362
2011-01-25 18:36:43fwierzbickisetnosy: + fwierzbicki
2011-01-25 18:09:27zyasoftsetpriority: urgent
severity: normal -> urgent
2011-01-25 17:35:06alex.gronholmcreate