Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch to use Python's tempfile library instead of 'temporary_file.temporary' #40

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jpellman
Copy link

@jpellman jpellman commented May 8, 2020

Under certain circumstances using the hardcoded path temporary_file.temporary can be problematic. For instance, if the starting directory for a Jupyter notebook instance is not writable by the user that started the notebook, trying to submit a fully typed out batch script will fail silently. There also may be contention issues in multi-user environments (i.e., a JupyterHub deployment) where multiple users start out in the same directory, since multiple users may try to write to the same temporary file at the same time.

This patch tries to mitigate these issues by using Python's built-in tempfile library to systematically create tempfiles that are exclusively/uniquely allocated to individual users in a location that is known to be writable. Since tempfile.mkstemp() returns an absolute path in /tmp (or the equivalent), there is no potential for an end-user to accidentally try to write to an unwritable directory.

@shreddd shreddd requested a review from rcthomas June 25, 2020 22:36
@shreddd
Copy link
Collaborator

shreddd commented Jun 25, 2020

This seems like a reasonable fix, and also addresses a race condition for multiple job submissions.

The one other addition I would suggest is that we may want these files sitting in the users home directory rather than /tmp. We could make that configurable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants