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

core: include AP handshake in 5s timeout #1458

Merged
merged 2 commits into from
Jan 28, 2025

Conversation

kingosticks
Copy link
Contributor

Inspired by #1456.

I bumped the timeout to 5s since we're doing more work now. And added a debug message so can tell if it's the connect or the handshake that died.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

roderickvd
roderickvd previously approved these changes Jan 25, 2025
Copy link
Member

@roderickvd roderickvd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, thanks.

@photovoltex
Copy link
Member

I can understand the log message, but why would we increase the timeout?

since we're doing more work now

I think the timeout was just recently introduced (~4 Months) and during that time nothing around connecting to the access point changed, I think. So is there really a need to bump the timeout there? I think it probably will not make a big difference, but I would probably like to know what you meant by "more work"^^

@kingosticks
Copy link
Contributor Author

kingosticks commented Jan 25, 2025

We previously had a 3 second timeout for just connecting the socket. This was an arbitrary time I plucked from thin air that seemed to me way longer than any real system would need. This change moves the AP handshake messaging into the timeout protection. So now we're trying to do socket connect and AP handshake in the same 3 second timeout period i.e. more work. Rather than needlessly break any users that relied on the full 3 seconds for just connecting (!), I just bumped it up to 5 seconds for both. This seemed safer whilst still being useful.

@photovoltex
Copy link
Member

Ah I see. Thanks for the clear up! I seem to have overlooked the move of the handshake.

@kingosticks
Copy link
Contributor Author

OK to merge?

@photovoltex
Copy link
Member

photovoltex commented Jan 28, 2025

Yes, just one last thought from my side. Would it make sense to make the timeout customizable?

@kingosticks
Copy link
Contributor Author

I don't think it's worth the bother but up to you.

@photovoltex
Copy link
Member

Sounds good to me. Only wanted to throw the idea into the room :)

@photovoltex photovoltex merged commit c1ae860 into librespot-org:dev Jan 28, 2025
13 checks passed
@kingosticks kingosticks deleted the handshake-timeout branch January 28, 2025 16:08
@kingosticks
Copy link
Contributor Author

Thanks

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.

3 participants