Skip to content

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.

2 participants