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
Milestone

Comments

@timkpaine
Copy link
Member

@timkpaine timkpaine commented Feb 26, 2018

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

@ian-r-rose ian-r-rose commented Feb 26, 2018

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

@timkpaine timkpaine commented Feb 26, 2018

@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

@ian-r-rose ian-r-rose commented Feb 27, 2018

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

@blink1073 blink1073 commented Mar 5, 2018

I think this might be addressed by #3687.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 16, 2018

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

@jasongrout jasongrout commented Mar 21, 2018

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

@timkpaine timkpaine commented Aug 20, 2018

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

@timkpaine
Copy link
Member Author

@timkpaine 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

@timkpaine timkpaine commented Aug 27, 2018

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

@ian-r-rose ian-r-rose commented Aug 27, 2018

@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

@timkpaine timkpaine commented Aug 27, 2018

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

@timkpaine timkpaine commented Aug 27, 2018

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

@timkpaine
Copy link
Member Author

@timkpaine 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

@timkpaine timkpaine commented Aug 27, 2018

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

@blink1073 blink1073 commented Aug 29, 2018

Fixed by #5224

@vidartf
Copy link
Member

@vidartf 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 vidartf commented Jul 29, 2019

(where the diff was applied)

@vidartf
Copy link
Member

@vidartf 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 removed this from the 0.32.0 milestone Oct 3, 2019
@timkpaine timkpaine added this to the 2.0 milestone Oct 3, 2019
@afshin
Copy link
Member

@afshin 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.

afshin added a commit to afshin/jupyterlab that referenced this issue Dec 24, 2019
@lock lock bot added the status:resolved-locked 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
Projects
None yet
6 participants