Skip to content
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

feat: prevent Electron versions that are in use by some window from being removed #1395

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
5 changes: 5 additions & 0 deletions src/interfaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -236,9 +236,14 @@ export type AppStateBroadcastMessage =
| {
type: AppStateBroadcastMessageType.syncVersions;
payload: RunnableVersion[];
}
| {
type: AppStateBroadcastMessageType.activeVersionsChanged;
payload?: never;
};

export enum AppStateBroadcastMessageType {
activeVersionsChanged = 'activeVersionsChanged',
isDownloadingAll = 'isDownloadingAll',
syncVersions = 'syncVersions',
}
Expand Down
3 changes: 2 additions & 1 deletion src/renderer/components/settings-electron.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ export const ElectronSettings = observer(
</ButtonGroup>
);
}

private filterSection(): JSX.Element {
const { appState } = this.props;
return (
Expand Down Expand Up @@ -401,7 +402,7 @@ export const ElectronSettings = observer(
break;
}

if (version === appState.currentElectronVersion.version) {
if (appState.activeVersions.has(version)) {
return (
<Tooltip
position="auto"
Expand Down
147 changes: 123 additions & 24 deletions src/renderer/state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,35 @@ export class AppState {
});
}

// Do we have a lock on the active Electron version that prevents other windows from removing it?
private hasActiveLock = false;

// Used to release the lock when the current window switches Electron versions
private versionLockController = new AbortController();

private static versionLockNamePrefix = 'version:';
erikian marked this conversation as resolved.
Show resolved Hide resolved

public activeVersions: Set<string> = new Set();

private getVersionLockName(ver: string) {
return `${AppState.versionLockNamePrefix}${ver}`;
}

/**
* Updates the Electron versions that are currently active in some window.
*/
private async updateActiveVersions(): Promise<void> {
this.activeVersions = ((await navigator.locks.query()).held || []).reduce<
Set<string>
>((acc, item) => {
if (item.name?.startsWith(AppState.versionLockNamePrefix)) {
acc.add(item.name.split(AppState.versionLockNamePrefix)[1]);
}

return acc;
}, new Set());
}

constructor(versions: RunnableVersion[]) {
makeObservable<AppState, 'setPageHash' | 'setVersionStates'>(this, {
Bisector: observable,
Expand All @@ -233,6 +262,7 @@ export class AppState {
addAcceleratorToBlock: action,
addLocalVersion: action,
addNewVersions: action,
activeVersions: observable,
channelsToShow: observable,
clearConsole: action,
currentElectronVersion: computed,
Expand Down Expand Up @@ -476,6 +506,12 @@ export class AppState {
const { type, payload } = event.data;

switch (type) {
case AppStateBroadcastMessageType.activeVersionsChanged: {
this.updateActiveVersions();

break;
}

case AppStateBroadcastMessageType.isDownloadingAll: {
this.isDownloadingAll = payload;
break;
Expand Down Expand Up @@ -795,34 +831,53 @@ export class AppState {
public async removeVersion(ver: RunnableVersion): Promise<void> {
const { version, state, source } = ver;

if (ver === this.currentElectronVersion) {
if (this.activeVersions.has(ver.version)) {
console.log(`State: Not removing active version ${version}`);
return;
}

console.log(`State: Removing Electron ${version}`);
if (source === VersionSource.local) {
if (version in this.versions) {
delete this.versions[version];
saveLocalVersions(Object.values(this.versions));
} else {
console.log(`State: Version ${version} already removed, doing nothing`);
}
} else {
if (
state === InstallState.installed ||
state == InstallState.downloaded
) {
await this.installer.remove(version);
if (this.installer.state(version) === InstallState.missing) {
await window.ElectronFiddle.app.electronTypes.uncache(ver);
await navigator.locks.request(
this.getVersionLockName(version),
{
mode: 'exclusive',
ifAvailable: true,
},
async (lock) => {
// another window is already removing this version
if (!lock) {
return;
}

console.log(`State: Removing Electron ${version}`);

this.broadcastVersionStates([ver]);
if (source === VersionSource.local) {
if (version in this.versions) {
delete this.versions[version];
saveLocalVersions(Object.values(this.versions));
} else {
console.log(
`State: Version ${version} already removed, doing nothing`,
);
}
} else {
if (
state === InstallState.installed ||
state == InstallState.downloaded
) {
await this.installer.remove(version);
if (this.installer.state(version) === InstallState.missing) {
await window.ElectronFiddle.app.electronTypes.uncache(ver);

this.broadcastVersionStates([ver]);
}
} else {
console.log(
`State: Version ${version} already removed, doing nothing`,
);
}
}
} else {
console.log(`State: Version ${version} already removed, doing nothing`);
}
}
},
);
}

/**
Expand Down Expand Up @@ -954,10 +1009,45 @@ export class AppState {
return;
}

if (this.hasActiveLock) {
console.log(`Releasing lock on version ${this.version}`);

// release the lock on the previous version
this.versionLockController.abort();

// replace the spent AbortController
this.versionLockController = new AbortController();
}

const { version } = ver;
console.log(`State: Switching to Electron ${version}`);
this.version = version;

navigator.locks.request(
erikian marked this conversation as resolved.
Show resolved Hide resolved
this.getVersionLockName(version),
{ mode: 'shared' },
(lock) => {
// let other windows know we're using this version
this.broadcastChannel.postMessage({
type: AppStateBroadcastMessageType.activeVersionsChanged,
});

// the current window's state also needs an update - that's how
// the current window knows it can't remove this version
this.updateActiveVersions();

this.hasActiveLock = Boolean(lock);

/**
* The lock is released when this promise resolves, so we keep it in the
* pending state until our AbortController is aborted.
*/
return new Promise<void>((resolve) => {
this.versionLockController.signal.onabort = () => resolve();
});
},
);

// If there's no current fiddle,
// or if the current fiddle is the previous version's template,
// then load the new version's template.
Expand All @@ -973,8 +1063,17 @@ export class AppState {
}
}

// Fetch new binaries, maybe?
await this.downloadVersion(ver);
await navigator.locks.request(
`downloading:${version}`,
{ mode: 'exclusive' },
async (lock) => {
console.log(`exclusive download lock granted:`);
console.log(lock);

// Fetch new binaries, maybe?
await this.downloadVersion(ver);
},
);
}

/**
Expand Down
3 changes: 3 additions & 0 deletions tests/mocks/state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { objectDifference } from '../utils';
export class StateMock {
public acceleratorsToBlock: BlockableAccelerator[] = [];
public activeGistAction = GistActionState.none;
public activeVersions = new Set<string>();
public channelsToShow: ElectronReleaseChannel[] = [];
public editorMosaic = new EditorMosaic();
public environmentVariables: string[] = [];
Expand Down Expand Up @@ -85,6 +86,7 @@ export class StateMock {
public setVersion = jest.fn().mockImplementation((version: string) => {
this.currentElectronVersion = this.versions[version];
this.version = version;
this.activeVersions.add(version);
});
public isVersionUsable = jest.fn().mockImplementation(() => {
return { ver: this.currentElectronVersion };
Expand Down Expand Up @@ -120,6 +122,7 @@ export class StateMock {
makeObservable(this, {
acceleratorsToBlock: observable,
activeGistAction: observable,
activeVersions: observable,
channelsToShow: observable,
editorMosaic: observable,
environmentVariables: observable,
Expand Down
10 changes: 8 additions & 2 deletions tests/renderer/components/settings-electron-spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,21 @@ import { ElectronSettings } from '../../../src/renderer/components/settings-elec
import { AppState } from '../../../src/renderer/state';
import { disableDownload } from '../../../src/renderer/utils/disable-download';
import { AppMock, StateMock, VersionsMock } from '../../mocks/mocks';
import { waitFor } from '../../utils';

jest.mock('../../../src/renderer/utils/disable-download.ts');

describe('ElectronSettings component', () => {
let store: StateMock;
let mockVersions: Record<string, RunnableVersion>;
let mockVersionsArray: RunnableVersion[];
const version = '2.0.1';

beforeEach(() => {
({ mockVersions, mockVersionsArray } = new VersionsMock());
({ state: store } = window.ElectronFiddle.app as unknown as AppMock);

store.initVersions('2.0.1', { ...mockVersions });
store.initVersions(version, { ...mockVersions });
store.channelsToShow = [
ElectronReleaseChannel.stable,
ElectronReleaseChannel.beta,
Expand All @@ -39,7 +41,7 @@ describe('ElectronSettings component', () => {
store.versionsToShow[i++].state = InstallState.installing;
});

it('renders', () => {
it('renders', async () => {
const spy = jest
.spyOn(window.ElectronFiddle, 'getOldestSupportedMajor')
.mockReturnValue(9);
Expand All @@ -65,6 +67,10 @@ describe('ElectronSettings component', () => {
const wrapper = shallow(
<ElectronSettings appState={store as unknown as AppState} />,
);

await store.setVersion(version);
await waitFor(() => store.activeVersions.size > 0);

expect(wrapper).toMatchSnapshot();

spy.mockRestore();
Expand Down
10 changes: 9 additions & 1 deletion tests/renderer/state-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@ import {
saveLocalVersions,
} from '../../src/renderer/versions';
import { VersionsMock, createEditorValues } from '../mocks/mocks';
import { overrideRendererPlatform, resetRendererPlatform } from '../utils';
import {
overrideRendererPlatform,
resetRendererPlatform,
waitFor,
} from '../utils';

jest.mock('../../src/renderer/versions', () => {
const { getReleaseChannel } = jest.requireActual(
Expand Down Expand Up @@ -311,6 +315,10 @@ describe('AppState', () => {

it('does not remove the active version', async () => {
const ver = appState.versions[active];

await appState.setVersion(ver.version);
await waitFor(() => appState.activeVersions.size > 0);

broadcastMessageSpy.mockClear();
await appState.removeVersion(ver);
expect(removeSpy).not.toHaveBeenCalled();
Expand Down
Loading