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

Conversation

erikian
Copy link
Member

@erikian erikian commented Jul 10, 2023

Some initial work on #1394:

2023-07-09_22-25-22.mp4

A couple call-outs:

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 ElectronSettings is unmounted because of the async state update (there's a comment in the code with more details).

Fixes #1394.

src/renderer/state.ts Outdated Show resolved Hide resolved
src/renderer/state.ts Outdated Show resolved Hide resolved
src/renderer/state.ts Show resolved Hide resolved
src/renderer/components/settings-electron.tsx Outdated Show resolved Hide resolved
src/renderer/components/settings-electron.tsx Outdated Show resolved Hide resolved
@erikian erikian force-pushed the feat/lock-versions-in-use branch 2 times, most recently from 116874c to 4398a4e Compare July 14, 2023 17:28
@dsanders11
Copy link
Member

dsanders11 commented Jul 15, 2023

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:

  1. Prevent clashing actions (removing while in use, simultaneous downloads, etc)
  2. 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 the ifAvailable option 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 on downloading:<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.

@erikian
Copy link
Member Author

erikian commented Jul 25, 2023

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):

  1. Open two Fiddle windows and their devtools.
  2. 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();
});
  1. 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):

  1. Open two Fiddle windows and their devtools.
  2. Open the version selector in both windows.
  3. 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();
});
  1. 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 🙂

@erikian erikian marked this pull request as ready for review July 25, 2023 02:55
@erikian erikian requested review from a team and codebytere as code owners July 25, 2023 02:55
@erikian erikian requested a review from dsanders11 August 2, 2023 00:11
@dsanders11 dsanders11 self-assigned this Aug 2, 2023
@erickzhao erickzhao marked this pull request as draft November 7, 2023 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent Removing Other Window Current Versions
2 participants