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

bug fix: timestamp changed but content did not change #15037

Closed
wants to merge 7 commits into from

Conversation

ctappy
Copy link

@ctappy ctappy commented Aug 29, 2023

References

Fixes #7743
Fixes #11352

Code changes

fixes timestamp changes but the content did not change. An issue with fuse filesystems, timing issue #7743, multiple users opening the same file #11352, and file touched but not edited issues.

User-facing changes

N/A

Backwards-incompatible changes

N/A

fixes timestamp changes but the content did not change. Issue with fuse filesystems, multiple users opening same file, and file touched but not edited issues.

Closes 7743 11352
@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@welcome
Copy link

welcome bot commented Aug 29, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@ctappy ctappy changed the title fix: timestamp changed but content did not change wip: timestamp changed but content did not change Aug 29, 2023
@ctappy ctappy marked this pull request as draft August 29, 2023 16:02
…heck object, order correctly and compare using JSON stringify
@ctappy ctappy changed the title wip: timestamp changed but content did not change bug fix: timestamp changed but content did not change Aug 29, 2023
@ctappy ctappy marked this pull request as ready for review August 29, 2023 17:23
@ctappy
Copy link
Author

ctappy commented Aug 29, 2023

I tested these changes with normal files and ipynb files

  • editing the file in the filesystem and saving it in JupyterLab
  • touching the file and saving it in JupyterLab

@krassowski krassowski added the bug label Aug 29, 2023
@krassowski
Copy link
Member

To fix the failing CI jobs please run integrity script and commit:

Please commit the changes by running:
git commit -a -m "Package integrity updates"

@krassowski
Copy link
Member

@jasongrout I see that you commented on both referenced issues, does this solution sound reasonable to you?

@ctappy ctappy changed the title bug fix: timestamp changed but content did not change wip: bug fix: timestamp changed but content did not change Aug 30, 2023
@ctappy ctappy changed the title wip: bug fix: timestamp changed but content did not change bug fix: timestamp changed but content did not change Aug 30, 2023
@ctappy ctappy changed the title bug fix: timestamp changed but content did not change WIP: bug fix: timestamp changed but content did not change Aug 31, 2023
@ctappy ctappy changed the title WIP: bug fix: timestamp changed but content did not change bug fix: timestamp changed but content did not change Aug 31, 2023
@ctappy
Copy link
Author

ctappy commented Aug 31, 2023

updated code to check object recursively and sort, then convert to a string for 1:1 comparison

@@ -681,7 +687,7 @@ export class Context<
): Promise<Contents.IModel> {
const path = this._path;
// Make sure the file has not changed on disk.
const promise = this._manager.contents.get(path, { content: false });
const promise = this._manager.contents.get(path, { content: true });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely would like to avoid pulling the content as comparing large file will take time and increase the wire load.

Is there a way to set better this.contentsModel?.last_modified?

Copy link
Author

@ctappy ctappy Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • we could add a comparison to files under a specific size and modification time difference

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to compute hashsum (e.g. md5) of the content on the server and on the frontend and compare the hashsums instead of comparing the contents directly?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fcollonval The problem with last_modified with fuse filesystems is not all of them handle the change/modified time correctly. If someone has their home directory mounted on a fuse filesystem this issue will constantly pop up, even though the file didn't change

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when the message comes up "File Changed" it is not technically correct, it should be "Timestamp Changed" because that is what the check is looking at, this would run a file change check

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krassowski I can't seem to find where the API call is going to where the file is opened. I'd love to add in a md5sum check if you can point me in the right direction. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be jupyter-server file contents API see https://github.com/jupyter-server/jupyter_server/blob/main/jupyter_server/files/ and https://jupyter-server.readthedocs.io/en/latest/developers/contents.html. It would probably be good to start by opening an issue on jupyter-server repository to discuss

FYI we ship a murmur2 function in the debugger package, so maybe we could reuse that for client-side check (if needed at all).

Copy link
Contributor

@Wh1isper Wh1isper Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we can compare the contents of the file on the server side and not manipulate the file if there is no change so that Timestamp doesn't change either.

@Wh1isper
Copy link
Contributor

I'm working on adding the md5 to the jupyter-server file contents API: jupyter-server/jupyter_server#1363

@Wh1isper
Copy link
Contributor

Wh1isper commented Nov 19, 2023

jupyter-server/jupyter_server#1363 has been merged, JupyterLab can now(next release actually) use md5 for file content verification!

cc @ctappy @fcollonval @krassowski

UPDATE:

md5 has been changed to hash and hash_algorithm in jupyter-server/jupyter_server#1367

Thank you all.

@krassowski
Copy link
Member

@ctappy or @Wh1isper would you be interested to rebase this PR including the new jupyter-server feature?

@krassowski krassowski added this to the 4.1.0 milestone Dec 20, 2023
@krassowski krassowski marked this pull request as draft December 20, 2023 17:59
@Wh1isper
Copy link
Contributor

@krassowski Thank you for the invitation. I’m interested but currently occupied with other commitments. Also, I’m relatively new to TypeScript and frontend development.

I may revisit this when I have some spare time! I’ll discuss this with a colleague who initially informed me about this PR. They might be interested in contributing.

@Wh1isper
Copy link
Contributor

Wh1isper commented Dec 29, 2023

Guys, I just created a PR that uses hash and hash_algorithm for file modification comparison: #15577

Forgive me for being a TypeScript and front-end novice, more advice would be appreciated, thanks!

cc @ctappy @krassowski @fcollonval

@krassowski
Copy link
Member

Closing in favour of #15577. Thank you all!

@krassowski krassowski closed this Dec 29, 2023
@Chralay Chralay mentioned this pull request Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants