Make reading from file handle thread safe

[eirki]

As mentioned in #342, using a global mutable variable to pass the file object to the read and seek callback functions causes an error if we try to read two files at once. Instead we can use use the io_ctx parameter of those two functions.

Let me know if you want me to make any changes to the PR.

(If this PR is disturbing your summer vacation, of course feel free to ignore it for now)

[ofajardo]

hi @eirki, thanks a lot for the PR! It looks very good!

There is just one thing I do not understand: in your test_read_sav_file_handle_threads, you say in your docstring that you want to test the file-like object reading in threads, but what I understand from your code is that you are first writing it to 10 different copies on disk and then reading those 10 copies on 10 threads, but from disk, not from an BytesIO object. So I got the feeling you are not testing the bug you corrected? If I am right, then I would suggest switching to 10 files like objects instead of reading from disk.

[eirki]

in your test_read_sav_file_handle_threads, you say in your docstring that you want to test the file-like object reading in threads, but what I understand from your code is that you are first writing it to 10 different copies on disk and then reading those 10 copies on 10 threads, but from disk, not from an BytesIO object. So I got the feeling you are not testing the bug you corrected? If I am right, then I would suggest switching to 10 files like objects instead of reading from disk.

The tests does indeed read from disk, but by passing an opened file (i.e. a file handle/file-like object) instead of passing the path, so

with open(path, "rb") as fo:
    df, meta = pyreadstat.read_sav(fo)

instead of

df, meta = pyreadstat.read_sav(path)

That means Pyreadstat has to deal with calling read and seek on the passed object. which is what (currently) relies on the global mutable. So I am reasonably sure that the test is testing the bug. We could achieve the same by reading the original file once and passing ten BytesIO objects, which (now that you mention it) is probably more elegant than writing ten files in a temp dir. Would like the me to update the PR?

[ofajardo]

Oh I see, thanks for the explamation. I think it would still be better to do it with BytesIO as it would be clearer for the reader what it is doing

[eirki]

Turns out reproducing the bug is a little harder using BytesIO - I'm guessing because reading from memory is much faster than reading from disk, so each thread finishes before it has any time to mess up the state in the other threads. It would of course be easy to reproduce using a dataset that takes longer to read, but requiring such large test datasets in the test suite is probably not good practice.

I was able to test for the bug by subclassing BytesIO and slowing down the read time:

    def test_read_sav_bytesio_threads(self):
        """Test reading SAV file from file-like object in multiple threads at once (tests thread safety)"""
        def read_sav_file(buffer):
            df, meta = pyreadstat.read_sav(buffer, output_format=self.backend)
            return df, meta

        class SlowBytesIO(io.BytesIO):
            """A BytesIO that sleeps a bit on each read."""
            def read(self, *args, **kwargs):
                time.sleep(0.1)
                return super().read(*args, **kwargs)

        sav_file = os.path.join(self.basic_data_folder, "sample.sav")
        with open(sav_file, "rb") as f:
            file_bytes = f.read()
        num_threads = 5
        buffers = [SlowBytesIO(file_bytes) for _ in range(num_threads)]
        with ThreadPoolExecutor(max_workers=num_threads) as executor:
            results = list(executor.map(read_sav_file, buffers))
        for df, meta in results:
            self.assertEqual(len(df.columns), len(self.df_pandas.columns))
            self.assertEqual(len(df), len(self.df_pandas))
            self.assertListEqual(list(df.columns), list(self.df_pandas.columns))

Do you prefer this solution?

[ofajardo]

@eirki Yes, that looks nice! Please implement that solution.