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

Call initHelpers on API check failure. #104

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

glogiotatidis
Copy link

Per #87 (comment) when a new player registers, ConnectDaemon correctly captures the event and creates a new helper but checkAPIConnectPlayers called right after will kill the process because it hasn't been registered to Spotify yet. A typical race condition.

This flow creates larger than needed delays to create a helper (and thus the player to appear in Spotify's interface). This PR will call initHelpers right after the helper is killed, to speed up the creation of a new one as soon as possible.

This has been tested and works as expected.

@michaelherger
Copy link
Owner

michaelherger commented Nov 19, 2023

But instead of checking players immediately after we've killed a player, shouldn't we delay the check which leads to the deletion in the first place?

Do you know what is calling checkAPIConnectPlayers() at this early stage, before status for the newly created player was available? It's my understanding that this would happen when we try to control the player. But this can be within a second, or minutes, we don't know.

@glogiotatidis
Copy link
Author

But instead of checking players immediately after we've killed a player, shouldn't we delay the check which leads to the deletion in the first place?

Not sure which flow triggers checkAPIConnectPlayers and didn't want to mess with this logic. While I understand your point about delaying the check on new register, my current approach feels more generic and in line with the logic of checkAPIConnectPlayers: If we kill a helper, then it makes sense to instantly try to recreate.

@michaelherger
Copy link
Owner

Ok then. But why would you still delay it using a timer? Couldn't we call the initialisation immediately?

@glogiotatidis
Copy link
Author

Fair point, will update to call directly.

@glogiotatidis
Copy link
Author

Updated @michaelherger

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.

None yet

2 participants