-
Notifications
You must be signed in to change notification settings - Fork 749
feat: prevent Electron versions that are in use by some window from being removed#1395
feat: prevent Electron versions that are in use by some window from being removed #1395erikian wants to merge 8 commits intoelectron:main from
Conversation
116874c to
4398a4e
Compare
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.
4398a4e to
96a196b
Compare
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:
2023年07月24日_23-25-46.mp4
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:
2023年07月24日_23-32-14.mp4
I think this is mostly ready for review, but I'd like some feedback on this before I add some new tests 🙂
ec48598 to
fd6d0f4
Compare
Some initial work on #1394:
2023年07月09日_22-25-22.mp4
A couple call-outs:
navigator.locks@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.
ElectronSettingsis unmounted because of the async state update (there's a comment in the code with more details).Fixes #1394.