feat: prevent Electron versions that are in use by some window from being removed
Some initial work on https://github.com/electron/fiddle/issues/1394:
https://github.com/electron/fiddle/assets/19478575/2290addc-79ab-4cfc-8210-b7b1805c6c21
A couple call-outs:
- this needs https://github.com/electron/fiddle/pull/1389 to land first, as the current Typescript version doesn't know about
navigator.locks - From the issue description:
Potential problem with this approach is navigator.locks.query is async, and the current UI doesn't handle async state. Looks like we could probably use react-async for the moment (with an upgrade from React v16.14 to v16.18) and then use Suspense in the future when React is upgraded to v18+.
@dsanders11 I used plain lifecycle methods to update the UI when a version is set as active in another window, so we might be able to do without another dependency here if that aligns with what you had in mind.
- upgrading to React 18 would be nice (though not necessary), there's an annoying "error" in development mode when the
ElectronSettingsis unmounted because of the async state update (there's a comment in the code with more details).
Fixes #1394.
I've been pondering it some more, and I think we should expand the usage of the locking as a general solution. There's two main things which need to be solved regarding versions and multiple windows:
- Prevent clashing actions (removing while in use, simultaneous downloads, etc)
- Update the UI to reflect which versions can be removed
The current state of this PR is solving 2, but I think we can solve 1 at the same time. Here's what I've been pondering:
- For removing, try to acquire an exclusive lock on the
version:<version>lock, but don't wait on the lock, use theifAvailableoption to fail fast- This is meant to handle the cases where one window is racing with the other: user clicks the remove button before the window updates the UI, or one window is doing "Remove All Downloads" and another window is changing active versions or downloading new versions
- When downloading, first get the shared
version:<version>lock, and then also try to get an exclusive lock ondownloading:<version>, waiting on the lock. That way we don't end up with simultaneous downloads, which can currently happen if you switch to an undownloaded version in a window and then open a new window.
I did what you suggested and came up with a way to simulate a user trying to do something simultaneously in multiple windows. It's a bit convoluted but the results seem to be reliable, so here it goes:
For the user clicks the remove button before the window updates the UI case, I did this (video below):
- Open two Fiddle windows and their devtools.
- In the first window, open Settings > Electron and create a BroadcastChannel in the console that will try to click on the "Remove" button for v25.3.0:
const receiverBroadcastChannel = new BroadcastChannel('testLocks');
receiverBroadcastChannel.addEventListener('message', async () => {
// the version is actually removed if we don't wait for a bit, but I assume
// users doing it manually would be at least this slow anyway :)
await new Promise(resolve => setTimeout(resolve, 20))
console.log('trying to remove the version the other window just selected');
const v25_3_0_removeButton = [
...document.querySelectorAll('table .bp3-button'),
][1];
v25_3_0_removeButton.click();
});
- In the second window, open the version selector, select the v25.3.0 button in the Elements tab in the devtools, and programatically click on it and create another BroadcastChannel that will post a message to the other window:
// $0 is the button that selects v25.3.0
$0.click();
const emitterBroadcastChannel = new BroadcastChannel('testLocks');
emitterBroadcastChannel.postMessage('whatever');
The first window will simulate the user clicking on the "Remove" button for 25.3.0 just after that version is selected in the second window, and nothing happens:
https://github.com/electron/fiddle/assets/19478575/f4db2aa6-b95e-4a8c-b08e-eacab8d0583f
For the simultaneous downloads case (video below):
- Open two Fiddle windows and their devtools.
- Open the version selector in both windows.
- In both devtools, create a BroadcastChannel in the console that will trigger the click on the first version that's not been downloaded - should be the same version in both windows:
const receiverBroadcastChannel = new BroadcastChannel('testLocks');
receiverBroadcastChannel.addEventListener('message', () => {
console.log('trying to download...');
document.querySelector('a[label="Not downloaded"]').click();
});
- In one of the devtools, create another BroadcastChannel that will post a message to the receiving BroadcastChannels in both windows:
const emitterBroadcastChannel = new BroadcastChannel('testLocks');
emitterBroadcastChannel.postMessage('whatever');
Both windows will receive the broadcast at the same time. Only one of the windows will be granted the lock at first (the window on the right side in the video), and the other window will only get the lock when the download finishes in the first window.
Even though the lock does its job, we still get an error and end up with an inconsistent state ("Checking status" in the download selector in the window that first got the lock, and a "dest already exists" error in the other window) when both windows try to download the same version in rapid succession (the same thing happens in the case you mentioned of the user opening a new window when a download is in progress), so maybe fiddle-core is not handling this properly based on the error stack:
https://github.com/electron/fiddle/assets/19478575/d7a4ba6a-00ea-4d16-88e4-6569671769d6
I think this is mostly ready for review, but I'd like some feedback on this before I add some new tests 🙂