-
Notifications
You must be signed in to change notification settings - Fork 37.7k
Remove IStateMainService #167086
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
Remove IStateMainService #167086
Conversation
| @IConfigurationService private readonly configurationService: IConfigurationService, | ||
| @ILogService private readonly logService: ILogService, | ||
| @IStateMainService private readonly stateMainService: IStateMainService | ||
| @IStateService private readonly stateMainService: IStateService |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you search/replace stateMain to state
| import { IUserDataProfile } from 'vs/platform/userDataProfile/common/userDataProfile'; | ||
| import { UserDataProfilesMainService } from 'vs/platform/userDataProfile/electron-main/userDataProfile'; | ||
| import { TestLifecycleMainService } from 'vs/platform/test/electron-main/workbenchTestServices'; | ||
| import { StateService } from 'vs/platform/state/node/stateService'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entire file should rename and move to node layer.
| } | ||
|
|
||
| export class InMemoryTestStateMainService implements IStateMainService { | ||
| export class InMemoryTestStateMainService implements IStateService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop main
| import { UserDataProfilesMainService } from 'vs/platform/userDataProfile/electron-main/userDataProfile'; | ||
| import { StateMainService } from 'vs/platform/state/electron-main/stateMainService'; | ||
| import { UriIdentityService } from 'vs/platform/uriIdentity/common/uriIdentityService'; | ||
| import { StateService } from 'vs/platform/state/node/stateService'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entire file should rename and move to node layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lramos15 actually now I remember why we have a state main service with read/write and a state service with just read capabilities: it is not very safe to allow for writes from the CLI when the main process is also running because you may end up with dirty write challenge: if the CLI manages to write while the main process is also running (e.g. you run CLI while VS Code is running), the main process will overwrite any changes the CLI made on shutdown.
Thus, node.js only has read access and main process read/write to prevent this.
Maybe a better fix would be this:
- use state service to read machine ID instead of accessing the file directly
- if machine ID is not there, look it up in the same way as main process does but do not write it back into state service
I think this is a good compromise.
|
Got it, will work on a better solution. |
Replace
IStateMainServicewithIStateServiceby expanding theIStateServiceinterface. Allows for usingIStateMainServicein more places