Skip to content

Conversation

@lramos15
Copy link
Member

Replace IStateMainService with IStateService by expanding the IStateService interface. Allows for using IStateMainService in more places

@lramos15 lramos15 enabled auto-merge (squash) November 23, 2022 18:34
@lramos15 lramos15 self-assigned this Nov 23, 2022
@lramos15 lramos15 requested a review from bpasero November 23, 2022 18:35
@vscodenpa vscodenpa added this to the November 2022 milestone Nov 23, 2022
@IConfigurationService private readonly configurationService: IConfigurationService,
@ILogService private readonly logService: ILogService,
@IStateMainService private readonly stateMainService: IStateMainService
@IStateService private readonly stateMainService: IStateService
Copy link
Member

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';
Copy link
Member

@bpasero bpasero Nov 24, 2022

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 {
Copy link
Member

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';
Copy link
Member

@bpasero bpasero Nov 24, 2022

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.

Copy link
Member

@bpasero bpasero left a 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.

@lramos15 lramos15 disabled auto-merge November 28, 2022 15:45
@lramos15 lramos15 closed this Nov 28, 2022
@lramos15
Copy link
Member Author

Got it, will work on a better solution.

@lramos15 lramos15 deleted the lramos15/acute-barnacle branch November 28, 2022 15:46
@github-actions github-actions bot locked and limited conversation to collaborators Jan 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants