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

Write notebook.json file atomically #3305

Merged
merged 1 commit into from Feb 6, 2018

Conversation

Projects
None yet
2 participants
@mdboom
Copy link
Contributor

mdboom commented Feb 5, 2018

The motivation for this was a bug in the jupyter-notebook-gist extension whereby a LazyConfigValue was being added to the JSON config tree in some cases. That's definitely a bug on the part of the extension that needs to be fixed over there.

However, this caused the notebook.json writer to crash mid-writing, leaving a corrupted notebook.json on disk, eg.:

{
  "load_extensions": {
    "jupyter-notebook-gist/extension": true,
    "jupyter-js-widgets/extension": true
  },
  "oauth_client_id":

So on subsequent startups of the Jupyter notebook, parsing fails unless the user manually fixes the JSON file. Potentially there could have been data loss in the config file if there were keys after oauth_client_id that failed to write.

I would argue that notebook shouldn't write the corrupted JSON to disk, but leave it in its last state if this happens. Manual testing shows that if a rogue extension runs, subsequent extensions are still run and can successfully add their own values to the JSON. In other words, only the data in single call to set or update is thrown out by this change.

I considered using the "write to a tempfile and replace" method for this. That's complicated to get correct in a cross-platform way (requires os.replace on Windows, which is only in Python 3.3 and later, so would require a backfill for Python 2.7 etc.) If streaming directly to disk is important, I'm happy to redo this that way, but I thought for config files that are generally quite small, doing this in-memory should be fine.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Feb 6, 2018

Thanks, I think this makes sense. The error on Appveyor is some npm problem, not related to this change.

@takluyver takluyver merged commit e250b0b into jupyter:master Feb 6, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jcb91 jcb91 referenced this pull request Mar 1, 2018

Closed

Problem enabling nbextensions #60

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.