Do not expect app.getPath('userData') to be there before app.once('ready')

[bpasero]

There is a discussion going on in 08a1700 where @chrmarti saw an issue on startup for new users that never started Code before. I never saw this issue but the fix seems odd to me: we now suddenly create the directory (userData) via mkdirp that is owned by Electron/Chrome, which seems wrong. It should be created by the framework, not us.

Can we move the getNodeCachedDataDir() into the app.once('ready') callback? That is the place where all code should go that assumes a certain directory structure to be present, not before.

[jrieken]

The idea is to run in parallel to the ready-event because if often takes a long time to be ready. @bpasero What is the issue here and what do you mean but is 'owned by Electron'? We are not locking or deleting the directory, just create it if its not there.

[bpasero]

This is how it used to be:

var dir = path.join(app.getPath('userData'), 'CachedData');
mkdir(dir);

Which is totally fine because we create the CachedData directory and we own it.

This is how @chrmarti changed it:

var dir = path.join(app.getPath('userData'), 'CachedData');
mkdirp(dir); // note this mkdirP

Which imho is not ok because we start to create directories all the way down until root if not existing (e.g. /Users/bpasero/Library/Application Support/code-oss-dev). We would do so in parallel with Electron/Chromium, because according to electron/electron#5340 the directory will be there inside the app.on('ready').

As such, we should move code into app.on('ready') that assumes certain Electron directories to be there.

[chrmarti]

To clarify why the change was done: Before when running with a new user data dir that did not exist yet, mkdir would fail because the parent folder (the user data dir) did not exist. As a result of that no window opened, that reproduced reliably on my machine when using --user-data-dir or moving my regular user data dir to a backup location. (bug #19529)

[bpasero]

@chrmarti so does it reproduce if you move that code into app.on(ready)?

[chrmarti]

No, it no longer reproduces.

[jrieken]

My motivation to move things before the ready-event is performance. It takes often takes 100-200ms before that event fires (avg. 293ms, 60th percentile 135ms). Our JS code doesn't do anything in that time because the event is generated outside the JS event loop (in native code). Today most (expensive) things like starting the shared process, loading files (backup, config, storage) happen after the ready-event resulting in a classic waterfall runtime behaviour. I didn't want to contribute to that waterfall by moving the cached data check after the ready-event but by letting it run in parallel. Also, I think we should move the afore mentioned things off the waterfall sequence and let them run in parallel.

@bpasero Is this just about us creating a folder that otherwise electron would create or about doing stuff before the ready-event? What problem do you see in us creating that folder and then electron not or vice versa?

Again, there are low hanging performance improvements and more code should be moved before the ready-event. Wrt operations that read files we can also not do them; there is no backup to read without a folder, byte code caching happens only on the second start etc...

Alternatively, we could refrain from creating the full path but only create the root dir if missing (code-oss-dev). That would likely help us with permission issues on non-standard paths and make us not walk up to root.

[bpasero]

I see the motivation. +1 for creating the root directory but not all the directories going up.

[chrmarti]

We decided to walk up the parent folders because there could be scenarios where users use --user-data-dir to point to a folder that needs its parent folder(s) created first.

[jrieken]

There is still a problem when mkdirp fails because of some unknown reason (permission?) in that case we found start up. I'll push a fix for that

[chrmarti]

Thought I had that covered. It should fail and print the associated error to the console.

[chrmarti]

@jrieken Just see your change now. I would definitely print the error to the console, otherwise any such problem will be very hard to track down.

We didn't know where the problem originated from and started with a binary search through our insiders builds to narrow down the change sets that could have caused the original problem. Then continued by looking at the diff stat and then diff between the two insider builds where it broke. Please log :)

[jrieken]

otherwise any such problem will be very hard to track down.

I generally agree but in this case it's not a problem. See, it's just an optional directory we pass on to the loader such that it can cache byte code. If that's missing everything should still work and startup can continue, it's just a little slower.

Once we move more and vital things here we should handle the error properly. Now I'd rely on electron doing that

[chrmarti]

I see, good for me then.