Improve ability of downstream apps to control file watching

[westbury]

What it does

This PR is a small refactoring that makes it easier for extenders to re-implement a file watching strategy.

As an example of a use-case, we have some very large libraries, pulled into sub-folders in projects as symbolic links. We do not want to listen to the library contents but we do want to know if a symbolic link has been changed to reference a different version of the library. We had code to optimize this, and we want to upgrade to include the file watching changes here: #8546

This involves the following changes:

1. Extenders may need to override functions in NsfwWatcher, as well as functions in NsfwFileSystemWatcherService. To make it easier to provide an implementation extending NsfwWatcher, I have extracted out createWatcher.
2. When the watcher is running as a subprocess, we cannot rebind in the process. The best we can do is create a replacement process. To make this easier, I have created a new option entryPoint.
3. I split the binding of FileSystemWatcherService (when not using a subprocess) into two, first bind the options, then bind the class itself.

It is not impossible for us to make our extensions without these changes. However it does make it easier and it avoids us
needing to duplicate chunks of code.

How to test

The code should work exactly the same as before, so test that file watchers still work correctly.

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[westbury]

@marechal-p thanks for the review. NsfwFileSystemWatcherServerOptions is definitely a better place than NsfwOptions. The only problem with it is that we are mixing options that are only used single threaded mode together with an option that only applies when using sub-processes, but that's better than mixing with NSFW options. I've made the change.

[paul-marechal]

@westbury CI is failing: https://github.com/eclipse-theia/theia/pull/9163/checks?check_run_id=2077838349#step:7:242

Sounds like it would be easier to dedicate a binding specifically for this entryPoint value after all.

[westbury]

@marechal-p yes, I tested my last change only in our application. If I had tested it also in the Electron example then I would have seen the problem. I could have fixed the build break simply by moving the binding of NsfwFileSystemWatcherServerOptions to before the singleThreaded test. However I think you are right that a separate set of options is cleaner. I've pushed a new commit.

[paul-marechal]

The change looks good to me, I just have one last comment:

I could have fixed the build break simply by moving the binding of NsfwFileSystemWatcherServerOptions to before the singleThreaded test.

Sorry for bothering you again, but I thought about this and my conclusion is that we should avoid conditional bindings. The code before always bound a given symbol, the branching is only meant to target a different implementation.

With your change, some symbols may or may not be bound. We can always do:

if (isBound(identifier)) {
    rebind(identifier)...
}

But I think those symbols should always be bound (NsfwFileSystemWatcherServerOptions and NsfwFileSystemWatcherProcessOptions). If never injected/requested they'll never be resolved.

WDYT?

[westbury]

@marechal-p don't worry about bothering me again, we need to get this right as it will be api. I see your point about having some things left unbound depending on the args. We do have different stuff bound depending on, for example, 'backend' vs 'backendElectron' but perhaps that is different.

Putting all the options together in one interface with them all optional seemed a little messy. I therefore put them in a discriminating union. I think that more precisely defines the options. Let me know what you think.

[paul-marechal]

We do have different stuff bound depending on, for example, 'backend' vs 'backendElectron' but perhaps that is different.

It is different in the way that if someone makes a backendElectron contribution then everything will be available in that context.

I therefore put them in a discriminating union.

So now to rebind it, one has to to a dynamic value to test processType?

rebind(FileWatcherProcessOptions).toDynamicValue(ctx => {
    const options = ctx.container.get(FileWatcherProcessOptions)
    if (options.processType === 'multi') {
        return { ... }
    } else {
        return { ... }
    }
});

My idea was to always bind both option objects, but conditionally use one or the other.

I've pushed a commit on top of your branch with a way I feel would work better? Feel free to either squash it or remove it.

[westbury]

@marechal-p sorry, I totally misunderstood your comment. So basically what we now have is, ignore my last change (back to b29bfaa) and

1. always bind both options interfaces. I understand your reason for wanting to do that.
2. extract out createNsfwFileSystemWatcherService and spawnNsfwFileSystemWatcherServiceProcess. Definitely a good idea as this makes the code more readable. Plus makes it easier if someone wants to rebind FileSystemWatcherService but only change one of the two alternatives.
3. Add the singleThreaded option as a parameter to bindFileSystemWatcherServer. This one I don't really understand. If an extender wants to change this option then surely they would just rebind FileSystemWatcherService. Unbinding everything so they can call bindFileSystemWatcherServer again with different options would be more complicated. However perhaps it is a good pattern that could be useful if other options were added, so this is fine by me.
4. exporting NSFW_SINGLE_THREADED and NSFW_WATCHER_VERBOSE. Thank you for thinking of this. Now I can replace our copy of this line with an import.

I have re-tested this for both single and multi-threaded. I cannot squash the commits because I can't sign your changes. They need to be merged as two separate commits. I have, however, taken the liberty of changing the message in your commit.

[paul-marechal]

Regarding 3) I don't understand either. The parameter was already there, I just slightly rewrote its definition... We can forget about this as I don't know what to make of it :)

I cannot squash the commits because I can't sign your changes. They need to be merged as two separate commits.

Do they? You should be able to squash/fixup your commit using git rebase?

Bash:

git rebase -i HEAD~2

Rebase todo-list:

pick <your commit>
fixup <my commit>

Once this is done if you are happy with the changes then I am happy too; I'll approve.

edit: If you worry about IP you could add the following line in your commit after squash:

Co-authored-by: Paul Maréchal <paul.marechal@ericsson.com>

[westbury]

@marechal-p I didn't think I was allowed to squash your commit because of #4279 (comment) . However co-authored seems to be allowed so I have added that and squashed the commits. Also removed the option parameter that is no longer useful.

Thanks for all your time helping with this.

[paul-marechal]

LGTM thanks!