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

Load extension host after workbench is running and introduce phase #38323

Closed
bpasero opened this Issue Nov 14, 2017 · 16 comments

Comments

Projects
None yet
4 participants
@bpasero
Member

bpasero commented Nov 14, 2017

I think we should try again to spawn the extension host only after the workbench is running. I remember @jrieken tried this but then we had clients that would prevent the restoring of editors from happening (via preferences editor).

We should replace IExtensionService.onReady() with a LifecyclePhase that is always fired after LifecyclePhase.Running imho and then we have a consistent picture of phases.

That means revisiting each user of IExtensionService.onReady() and making sure it is not blocking the startup. One challenge is that it is hard to prevent deadlocks from happening because we will always add new code that might cause it.

Thoughts?

@bpasero bpasero changed the title from Load extension host after workbench is running and introduce event to Load extension host after workbench is running and introduce phase Nov 14, 2017

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 14, 2017

Member

@sandy081 problematic in settings/keybindings: KeybindingsEditorModel.resolve() and PreferencesService.resolve()

Potentially dangerous (depends on usage): IExtensionService.getExtensions() and IExtensionService.readExtensionPointContributions()

And then a lot in IModeService: getOrCreateMode, getOrCreateModeByLanguageName, getOrCreateModeByFilenameOrFirstLine, et al

Member

bpasero commented Nov 14, 2017

@sandy081 problematic in settings/keybindings: KeybindingsEditorModel.resolve() and PreferencesService.resolve()

Potentially dangerous (depends on usage): IExtensionService.getExtensions() and IExtensionService.readExtensionPointContributions()

And then a lot in IModeService: getOrCreateMode, getOrCreateModeByLanguageName, getOrCreateModeByFilenameOrFirstLine, et al

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Nov 14, 2017

Member

Thoughts?

We should remove IExtensionService#onReady. Maybe make it a lifecycle event but surely never block on it but treat it like an event, e.g. to update something

Member

jrieken commented Nov 14, 2017

Thoughts?

We should remove IExtensionService#onReady. Maybe make it a lifecycle event but surely never block on it but treat it like an event, e.g. to update something

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 14, 2017

Member

@jrieken yeah I had that thought as well, if we make it as an event it reduces the chance that someone would chain that promise up all the way to some critical code path.

However, if we have an event, that would be an argument against adding it to the lifecycle service, because there we typically have promises.

Problem I see is with the mode service things blocking on the extensions to be ready. Maybe @alexandrudima could chime in if that could also be event based and not blocking.

Member

bpasero commented Nov 14, 2017

@jrieken yeah I had that thought as well, if we make it as an event it reduces the chance that someone would chain that promise up all the way to some critical code path.

However, if we have an event, that would be an argument against adding it to the lifecycle service, because there we typically have promises.

Problem I see is with the mode service things blocking on the extensions to be ready. Maybe @alexandrudima could chime in if that could also be event based and not blocking.

@sandy081

This comment has been minimized.

Show comment
Hide comment
@sandy081

sandy081 Nov 14, 2017

Member

Fixed it in Settings editor by removing the dependency on IExtensionService#onReady instead listening to default configuration changes - #38329

Member

sandy081 commented Nov 14, 2017

Fixed it in Settings editor by removing the dependency on IExtensionService#onReady instead listening to default configuration changes - #38329

@sandy081

This comment has been minimized.

Show comment
Hide comment
@sandy081

sandy081 Nov 14, 2017

Member

Re KeybindingsEditorModel.resolve(), Keybindings editor renders Keybindings and unbound commands. There is already an event that is triggered when keybindings are changed / registered. But not sure there is an event triggered when commands change / registered so that the editor can be updated.

Member

sandy081 commented Nov 14, 2017

Re KeybindingsEditorModel.resolve(), Keybindings editor renders Keybindings and unbound commands. There is already an event that is triggered when keybindings are changed / registered. But not sure there is an event triggered when commands change / registered so that the editor can be updated.

@alexandrudima

This comment has been minimized.

Show comment
Hide comment
@alexandrudima

alexandrudima Nov 14, 2017

Member

@bpasero IModeService.getOrCreateMode() blocks on the extensions ready (i.e. extensions scanned and extension points invoked, not necessarily extension host process ready), but that is IMHO inconsequential as it does not block the creation/displaying of a buffer.

Member

alexandrudima commented Nov 14, 2017

@bpasero IModeService.getOrCreateMode() blocks on the extensions ready (i.e. extensions scanned and extension points invoked, not necessarily extension host process ready), but that is IMHO inconsequential as it does not block the creation/displaying of a buffer.

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 14, 2017

Member

@sandy081 well, the idea is to not join your promise on the extensions ready promise. So the event that you are looking for is still the same ("extensions ready") but you should simply update the editor when it happens instead of waiting for it before the editor is declaring to be loaded.

...I think all of this will be much easier to not do wrong once we replace the promise to be an event.

Member

bpasero commented Nov 14, 2017

@sandy081 well, the idea is to not join your promise on the extensions ready promise. So the event that you are looking for is still the same ("extensions ready") but you should simply update the editor when it happens instead of waiting for it before the editor is declaring to be loaded.

...I think all of this will be much easier to not do wrong once we replace the promise to be an event.

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Nov 14, 2017

Member

There is already an event that is triggered when keybindings are changed / registered.

Really, where? I'd copy from it

But not sure there is an event triggered when commands change / registered so that the editor can be updated.

Doesn't exist but can be added

Member

jrieken commented Nov 14, 2017

There is already an event that is triggered when keybindings are changed / registered.

Really, where? I'd copy from it

But not sure there is an event triggered when commands change / registered so that the editor can be updated.

Doesn't exist but can be added

@sandy081

This comment has been minimized.

Show comment
Hide comment
@sandy081
Member

sandy081 commented Nov 14, 2017

@sandy081

This comment has been minimized.

Show comment
Hide comment
@sandy081

sandy081 Nov 14, 2017

Member

the idea is to not join your promise on the extensions ready promise

@bpasero Not sure what you mean here. Settings editor is no longer depending on the extensions ready promise. Also it does not wait for any. It simply updates the editor with newly registered default settings by listening to the configuration change event. More details are here #38329.

I would like to do the same for Keybindings editor. But I am missing an event when commands are registered.

Member

sandy081 commented Nov 14, 2017

the idea is to not join your promise on the extensions ready promise

@bpasero Not sure what you mean here. Settings editor is no longer depending on the extensions ready promise. Also it does not wait for any. It simply updates the editor with newly registered default settings by listening to the configuration change event. More details are here #38329.

I would like to do the same for Keybindings editor. But I am missing an event when commands are registered.

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 15, 2017

Member

@sandy081 makes sense. I was just saying that as a workaround for now you could still react to the extensions loading promise. But in general if we ever want to support installing an extension without window reload then this would not be a good approach, so an event that is specific is always better 👍

Member

bpasero commented Nov 15, 2017

@sandy081 makes sense. I was just saying that as a workaround for now you could still react to the extensions loading promise. But in general if we ever want to support installing an extension without window reload then this would not be a good approach, so an event that is specific is always better 👍

@sandy081

This comment has been minimized.

Show comment
Hide comment
@sandy081

sandy081 Nov 15, 2017

Member

Agreed. We just have specific events rather than general extensions loading promise.

Member

sandy081 commented Nov 15, 2017

Agreed. We just have specific events rather than general extensions loading promise.

@sandy081 sandy081 closed this Nov 15, 2017

@sandy081 sandy081 reopened this Nov 15, 2017

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 15, 2017

Member

@sandy081 could you introduce a new event from IExtensionService that signals extensions have changed? This event could then also be sent when we enable/disable extensions during the session without restart and adopters would be aware that this can happen anytime (like root folders can change anytime even after startup). I see in many cases the current promise is used like an event already.

Member

bpasero commented Nov 15, 2017

@sandy081 could you introduce a new event from IExtensionService that signals extensions have changed? This event could then also be sent when we enable/disable extensions during the session without restart and adopters would be aware that this can happen anytime (like root folders can change anytime even after startup). I see in many cases the current promise is used like an event already.

@bpasero bpasero added this to the November 2017 milestone Nov 15, 2017

@sandy081

This comment has been minimized.

Show comment
Hide comment
@sandy081

sandy081 Nov 15, 2017

Member

Sure

Member

sandy081 commented Nov 15, 2017

Sure

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 15, 2017

Member

@sandy081 I still think we should then maybe go and check for each consumer if a more fine grained event would make sense and would be possible. That could come as a second step.

Member

bpasero commented Nov 15, 2017

@sandy081 I still think we should then maybe go and check for each consumer if a more fine grained event would make sense and would be possible. That could come as a second step.

bpasero added a commit that referenced this issue Nov 21, 2017

@bpasero bpasero closed this in c8b4dfd Nov 21, 2017

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 21, 2017

Member

Things pushed:

  • extension host created in running phase (previously restored)
  • renamed onReady to whenInstalledExtensionsRegistered
  • introduced a new event onDidRegisterExtensions which will fire after the initial extensions are registered as well when we ever enable to enable extensions during runtime without window reload

I was able to replace whenInstalledExtensionsRegistered with the event in some places but had to leave it in for other places. When we approach the topic of allowing to enable extensions without window reload we need to basically revisit each place of whenInstalledExtensionsRegistered and adopt the event as well.

I did a check of all remaining users of whenInstalledExtensionsRegistered and verified that none would block the startup, so I think we should be OK.

Member

bpasero commented Nov 21, 2017

Things pushed:

  • extension host created in running phase (previously restored)
  • renamed onReady to whenInstalledExtensionsRegistered
  • introduced a new event onDidRegisterExtensions which will fire after the initial extensions are registered as well when we ever enable to enable extensions during runtime without window reload

I was able to replace whenInstalledExtensionsRegistered with the event in some places but had to leave it in for other places. When we approach the topic of allowing to enable extensions without window reload we need to basically revisit each place of whenInstalledExtensionsRegistered and adopt the event as well.

I did a check of all remaining users of whenInstalledExtensionsRegistered and verified that none would block the startup, so I think we should be OK.

@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 5, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.