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

Issue with /lab/tree #4009

Closed
timkpaine opened this issue Feb 26, 2018 · 19 comments · Fixed by #7676 or #7695
Closed

Issue with /lab/tree #4009

timkpaine opened this issue Feb 26, 2018 · 19 comments · Fixed by #7676 or #7695
Assignees
Labels
bug pkg:filebrowser status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Milestone

Comments

@timkpaine
Copy link
Member

If you have a lot of stuff in the top level, and navigate to /lab/tree/a/sub/directory, the subdirectory loads in the file browser, then it redirects to the top directory. I think this is because the calls are asynchronous, and getting the contents of the subdirectory finishes before the top directory, but the other calls should be ignored if the url is /lab/tree.

@ian-r-rose
Copy link
Member

Thanks for the report @timkpaine. I think you are correct: there is probably a race condition between the restoration of the filebrowser cwd and the tree handler.

@ian-r-rose ian-r-rose added this to the Beta 2 milestone Feb 26, 2018
@timkpaine
Copy link
Member Author

@ian-r-rose thanks! its especially noticeable for us because the tree handler default path has like 4 items, but the top level has literally 4000, so its like 2 seconds after page load the directory changes.

@ian-r-rose
Copy link
Member

I've been trying to reproduce this with a directory that has 5000 files, and a subdirectory with just one. It seems to be working correctly, as far as I can tell. Is there any other information that you can think of that would be relevant here, @timkpaine?

@blink1073
Copy link
Member

I think this might be addressed by #3687.

@jasongrout
Copy link
Contributor

Ping @timkpaine - #3687 is now merged into master. Can you see if this is still an issue with that PR merged?

@jasongrout
Copy link
Contributor

Closing as resolved. Please add a new comment if we still need to do something or to continue the discussion.

@timkpaine
Copy link
Member Author

This is still an issue, let me write a script to generate a directory structure to simulate it

@timkpaine
Copy link
Member Author

timkpaine commented Aug 21, 2018

here is a repro, it will put you in the wrong folder the first time, correct folders on subsequent calls to /lab/tree/test2

#!/bin/bash


mkdir test
cd test

echo "c.NotebookApp.default_url = '/login?next=/lab/tree/test2'" >>test.cfg


for i in `seq 1 10000`; do
        touch $i.py
done    

mkdir test2
cd test2
touch test.py

cd ../
jupyter lab --config=./test.cfg

@ian-r-rose ian-r-rose reopened this Aug 21, 2018
@timkpaine
Copy link
Member Author

More info, the initial launcher is in the correct sub directory, it's just the file browser that is wrong

@ian-r-rose
Copy link
Member

@timkpaine I am still having a hard time reproducing this with your script. Can you confirm that it is still an issue with 0.34.3?

@timkpaine
Copy link
Member Author

yes. I'm poking around in the debugger, it looks like the default file browser created here doesn't have any of the path information (I.e. /lab/tree/), while by the time you get here the browser.model now has the path info.

@timkpaine
Copy link
Member Author

basically, its possible for the browser model's path to not match what it is showing

@timkpaine
Copy link
Member Author

timkpaine commented Aug 27, 2018

heres the sequence of the cd function being called:

  • called with '.'
  • called with 'my path'
  • hit here with 'my path'
  • hit there with '.', but oldValue is still equal to '', so this clobbers the changes the previous hit had

@timkpaine
Copy link
Member Author

from gitter:
refresh timer starts to cd . right away: https://github.com/jupyterlab/jupyterlab/blob/master/packages/filebrowser/src/model.ts#L107

may race condition with initial navigate

@blink1073
Copy link
Member

Fixed by #5224

@vidartf
Copy link
Member

vidartf commented Jul 29, 2019

Just tested this locally by adding a delay to the contents API:

--- a/packages/services/src/contents/index.ts
+++ b/packages/services/src/contents/index.ts
@@ -1022,6 +1022,16 @@ export class Drive implements Contents.IDrive {

     let settings = this.serverSettings;
     return ServerConnection.makeRequest(url, {}, settings)
+      .then(response => {
+        if (!options.content) {
+          return response;
+        }
+        return new Promise<Response>(resolve =>
+          setTimeout(() => {
+            resolve(response);
+          }, 5000)
+        );
+      })
       .then(response => {
         if (response.status !== 200) {
           throw new ServerConnection.ResponseError(response);

If I have a session where the file browser is sitting in subdir/a, and I then navigate (via URL) to /tree/some/path the following calls are made (according to network tab in chrome devtools):

  • api/contents/?content=1 (root dir)
  • api/contents/subdir/a (previous session dir)
  • api/contents/some/path?content=0 (URL navigation dir)
  • api/contents/subdir/a?content=1 (previous session dir, dir listing)
  • api/contents/some/path?content=0 (URL navigation dir)
  • api/contents/some/path?content=1 (URL navigation dir, dir listing)

With the above delay, this will take 15s+ to get the dir listing from the URL arg.

@vidartf
Copy link
Member

vidartf commented Jul 29, 2019

(where the diff was applied)

@vidartf
Copy link
Member

vidartf commented Jul 29, 2019

Ideal resolution:

  • When a tree URL is given, have the file browser navigate directly there on load (assuming it is a valid path).
  • Without a (valid) tree URL, have the file browser navigate directly to the previous session dir on load (assuming it is still a valid path).
  • Otherwise navigate to server default dir (/).

In all cases, there should only be one API call with content=1.

@timkpaine timkpaine modified the milestones: 0.32.0, 2.0 Oct 3, 2019
@afshin
Copy link
Member

afshin commented Dec 24, 2019

I think the race condition is fixed, but the initial load still happens. I think the file browser may need some refactoring to prevent its initial API request, so while the current fix is an improvement, it's not a complete fix.

I will look at this before 2.0, but it may need to be pushed to a later release.

@lock lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Feb 1, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug pkg:filebrowser status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
6 participants