Make #107 compatible and found HttpReadStream resource leak #117
Conversation
|
Thanks for investigating closing but the most important of this PR is that new unit tests need to be added when some gzipped/bz2 file is opened via http. |
rctzeng
changed the title
|
OK, my bad, I've added unit tests for read/write mode tests and close tests for bz/gz2 over http.
the unit test for file-handle leak tests are commented. |
| return make_closing(BZ2File)(filename, mode) | ||
|
|
||
| elif ext == '.gz': | ||
| from gzip import GzipFile | ||
| if 'http' in root: | ||
| return make_closing(HttpReadStream)(filename, mode) | ||
| return make_closing(GzipFile)(filename, mode) |
There was a problem hiding this comment.
This doesn't fix the resource leak -- it just ignores the compression extension and then re-opens the URL, downloading it a second time. That's not correct.
| @responses.activate | ||
| def test_read_write_http_gz(self): | ||
| """Does modes correctly for gz over http?""" | ||
| text= 'line1\nline2' |
There was a problem hiding this comment.
this is not gzipped content. Please test with gzipped content. There are examples of archived files in https://github.com/RaRe-Technologies/smart_open/tree/master/smart_open/tests/test_data
| @responses.activate | ||
| def test_read_write_http_bz2(self): | ||
| """Does modes correctly for bz2 over http?""" | ||
| text= 'line1\nline2' |
There was a problem hiding this comment.
This is not bz2 content. Please use actual bz2 content
| @responses.activate | ||
| def test_close_http_gz(self): | ||
| """Is gzip over http closed correctly?""" | ||
| text= 'line1\nline2' |
| @responses.activate | ||
| def test_close_http_bz2(self): | ||
| """Is bz2 over http closed correctly?""" | ||
| text= 'line1\nline2' |
| self.assertRaises(ValueError, fin.read) | ||
|
|
||
| if os.path.isfile(test_file): | ||
| os.unlink(test_file) | ||
|
|
There was a problem hiding this comment.
This doesn't test that a filehandle hasn't been closed -- it just tests if fin.read honors that the filehandle isn't closed. You could argue that HttpReadStream should raise ValueError if someone tries to read() after close() has been called, but the fact that it allows it isn't, by itself, a resource leak.
|
Fixed in #112 |
Hello, I'm Ruo-Chun Tzeng, who'd like to participant in GSoC 2017.
As a test, I was assigned to fix #112.
There're 2 changes:
and caught gzip, bz2 are not properly closed on python 2.6.9 (failed my file-closing test)
make_closingfunction. so that HttpReadStream work with compressed formatsHowever, I found the HttpReadStream is not properly closed, the contents is still readable after closing the stream.