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

Multiple fix #32

Closed
wants to merge 3 commits into from
Closed

Multiple fix #32

wants to merge 3 commits into from

Conversation

sproctor
Copy link
Contributor

@sproctor sproctor commented Jul 9, 2024

The protection against multiple concurrent usages was broken to never allow restarting. This change is pretty significant, but also simple. LighthouseState is no longer a member variable, instead a new state is created on each call to LighthouseClient.discoverDevices(). shouldPersist is no longer possible to implement. If that feature is required, an existing list could be passed to discoverDevices.

I removed DiscoveryManager. It didn't actually manage anything anymore. The two helper functions from there made more sense as part of RealLighthouseClient.

Fixes #31

@sproctor
Copy link
Contributor Author

sproctor commented Jul 9, 2024

One last thing to note, discoverDevices now runs on the current coroutine context and the proper context should be set by the consumer.

@ivanempire
Copy link
Owner

ivanempire commented Jul 9, 2024

Two quick notes:

  1. Is the idea to have .flowOn() at the consumer level? EDIT: I think part of the reason I added it there was to make sure IO is used and to make it possible to add one during testing....which I totally wrote since xD
  2. Won't the removal of the mutex cause an issue where the second call to discoverDevices() throws an error up the Flow?

@ivanempire
Copy link
Owner

Closing for now since we're both async on this work - feel free to reopen :)

@ivanempire ivanempire closed this Sep 23, 2024
@sproctor
Copy link
Contributor Author

  1. Deciding which dispatcher to use should be left to the user.

  2. A new LighthouseState is created on each discoverDevices() call, so it avoids the issues with re-use. I don't remember if I tried concurrent searches. If you're interested in this work I can test it and make any relevant fixes.

You don't seem keen on merging my stuff, so unless you tell me otherwise, I'll just leave stuff on my own branch. It'll save me the work of trying to make things mostly mergeable (although, I did make a lot of my merges dependent on previous ones because I didn't have the time to break them up completely).

@ivanempire
Copy link
Owner

Nah I'm cool with it, I just want smaller commits/also been taking a break. You're all good, I just also don't have the best environment for testing on desktop - I swear I'll get to it xD

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.

Re-starting a discovery causes a crash
2 participants