search

ucphhpc / migrid-sync

MiGrid workspace where master branch is kept strictly in sync with SF upstream svn repo. Any development or experiments should use a branch. You probably want to fork your own clone or work e.g. on the edge branch if you wish to contribute.
GNU General Public License v2.0
3 stars 4 forks source link

Address filehandle leaking across file writing methods in fileio. #48

Closed albu-diku closed 4 months ago

albu-diku commented 4 months ago

The existing code places a try/catch around both the write() and close() rather than closing in a finally - the effect of that being that if the write raised an exception close would not be called. This was noticed when running the code under test produced ResourceWarning messages (something that is now a hard error in the test support code now it matches these).

Fix this by jumping straight to a standard context manager syntax. Given the issue existed for both chunk and whole file writing, and having noted that those functions were nearly identical, refactor whole file writes in terms of chunk writes thus it inherits the fixes. Cover all of this.

jonasbardino commented 4 months ago

Thanks, manually merged the test. Do you want to attempt a fix in fileio, too? I'll leave this PR open for now.

albu-diku commented 4 months ago

@jonasbardino this should address all the cases in the write methods for now (and means they should all be covered). As an aside, the workflows not being run on PRs until approval seems like a misconfiguration.. almost certainly we'd like to know that something passed its own tests as input to a review :)

jonasbardino commented 4 months ago

Hmm, I started manually merging through svn, but ran into some issues with the make PY=2 test in your PR branch.

Namely, python2 chokes on the asterisk (*) in _write_chunk args, and even if I remove it the make PY=2 test fails with IOError: [Errno 2] No such file or directory: '/usr/src/app/tests/output/fileio/write_file'. Did I miss something?

It's rather annoying with the PR workflow missing, indeed. I'm not familiar with GitHub Actions, and gave up debugging due to more urgent matters.

albu-diku commented 4 months ago

@jonasbardino have fixed the issue (mode definition ordering for PY2) and improved the commit message to boot :)

jonasbardino commented 4 months ago

Manually merged through svn - with quite a few follow-up changes after I realized how much it really influences the logic in experimental. Refactored the included automatic mode selection code for handling the case where byte strings are attempted written to plain text mode opened files into a shared _auto_adjust_mode helper. It's now used both in write_chunk and write_file and is extended to cover the opposite case of trying to write unicode strings to files opened in binary mode as well. I've added a bunch of new test cases and removed some of the previous divergence with explicit conversion of input data on experimental now that we select the proper mode purely based on input data type instead. In the end I think this might turn out to be the better approach and potentially allow us eliminate the force utf8 and native string conversion during I/O.