Thread safety concerns with reliance on nibabel.tmpdirs.InTemporaryDirectory

[jacobdr]

I opened a similar ticket against dipy but figured it would be useful to open here as well as nibabel is the root cause...

Reading through the nibabel source code for InTemporaryDirectory its doing some quite sketchy things like:

self._pwd = os.getcwd()
os.chdir(self.name)

To support programs that are mutlithreaded -- as a lot of API server implementations are in the Python universe -- I suggest the implementation be revisited so as to not accidentally introduce race conditions into the code of consuming applications.

I can work on a reproduction but logically since Python threads share a single process and os.chdir operates at the process level, conceptually I think its obvious nibabel is making some bad assumptions

[effigies]

Hi, thanks for this. I'm not really clear on what you're proposing. I don't think there's really a way to change the implementation so that races can't occur... changing the process working directory is subject to races, so you shouldn't use this in a multi-threaded context.

Are you suggesting we deprecate this public API because it's being misused?

[matthew-brett]

Yes, I think we're only using this in the tests, where I don't think it's controversial. I guess we could add something to the docstring to point out changing directory is not thread-safe?

[jacobdr]

The docstring approach I think is a good short-term mitigation but I think i would like to see a longer-term solution.

The deprecation notice for a future major is a great proposal (I would be happy to support dipy in its removal). An even better one would be a non-breaking thread-safe implementation but nothing immediately comes to mind there.

[jacobdr]

@effigies slightly off topic but I saw there was some prior art to move to pytest already done but the Makefile still uses nosetests as the runner... what is the story with that? (trying to run the tests locally now)

[effigies]

I think I forgot to look at the Makefile when we switched to pytest.

[jacobdr]

@effigies do you want me to take a crack at that in a separate ticket number and match what is done in the gitlab CI workflow?

[effigies]

Sure!

[matthew-brett]

I don't think there's any reason to deprecate the class, because it is useful for testing. And I find it difficult to imagine a another implementation of the class that changes directory in a Python process that doesn't cause threading issues. So my vote would be to leave the code as is, but be careful to think it through if we're considering using it in the main codebase, instead of the tests.

[effigies]

My thought was that we might move it into the testing submodule to more clearly signal intended use. But it is a general utility that could have uses in interactive sessions, so that might be going a bit far.