Skip to content

fix: delegate starting receiver loops to caller - #69

Merged
devgianlu merged 10 commits into
devgianlu:masterfrom
silenium-dev:master
Aug 20, 2024
Merged

devgianlu merged 10 commits into
devgianlu:masterfrom
silenium-dev:master

Conversation

@silenium-dev

Copy link
Copy Markdown
Contributor

fixes #68

@devgianlu

Copy link
Copy Markdown
Owner

The reason why the receiver isn't started is that there might be messages that arrive immediately after connection and if there's no listener then they'll be lost.

You'll need to call ReceiveMessage or ReceiveRequest and read from the channel they return. Or find an actual fix?

@silenium-dev

silenium-dev commented Aug 18, 2024 •

Copy link
Copy Markdown
Contributor Author

Maybe we could start the ping loop on the first ReceiveMessage/ReceiveRequest call? Because it shouldn't be started either, if there's no receiver that could process the pongs.
Or we even could open the websocket connection on the first call, instead of immediately on creation.

@devgianlu

Copy link
Copy Markdown
Owner

Maybe we could start the ping loop on the first ReceiveMessage/ReceiveRequest call? Because it shouldn't be started either, if there's no receiver that could process the pongs.

That makes sense, but I believe that the server will close the connection if we don't ping it.

Or we even could open the websocket connection on the first call, instead of immediately on creation.

This makes even more sense, but we must be careful to the fact that the connection will be nil.

Because connection can fail, I don't think delegating it to the Receive* methods is feasible. Instead it should probably be handled by the caller (outside of NewDealer) through an exported Connect method, but we must deal with nil conn pointers and the reconnectLock (which should probably be named connMu).

All of the above also applies to the Accesspoint.

@silenium-dev

Copy link
Copy Markdown
Contributor Author

I'm going to reopen this when I've implemented the proper fix

@silenium-dev silenium-dev reopened this Aug 19, 2024
@silenium-dev
silenium-dev marked this pull request as draft August 19, 2024 20:04
@silenium-dev
silenium-dev marked this pull request as ready for review August 20, 2024 06:24
@silenium-dev

Copy link
Copy Markdown
Contributor Author

I'm unsure whether to move Accesspoint.Connect() out of Session as AudioKeyProvider and Spclient depend on it.
Nothing internal depends on Dealer, so moving Connect() out of Session seems ok.

@silenium-dev

silenium-dev commented Aug 20, 2024 •

Copy link
Copy Markdown
Contributor Author

It seems like the dealer ping timings are a bit too tight, it sends a ping every 30s, but also has a timeout of 30s, which can cause some network latency related reconnects, but that's only a guess, because it only happens on the 5th ping (after 150s) and I'm still looking into it

@silenium-dev

silenium-dev commented Aug 20, 2024 •

Copy link
Copy Markdown
Contributor Author

Seems to be fixed by setting the time that the pong can be behind to pingInterval + timeout.
lastPong is now reset on reconnect to prevent a reconnect loop.

@silenium-dev silenium-dev changed the title fix: start dealer receive loop on dealer creation fix: delegate starting receiver loops to caller Aug 20, 2024
Comment thread dealer/dealer.go Resolved
Comment thread dealer/dealer.go Resolved
Comment thread dealer/dealer.go Resolved
Comment thread ap/ap.go Resolved
@devgianlu

Copy link
Copy Markdown
Owner

I'm unsure whether to move Accesspoint.Connect() out of Session as AudioKeyProvider and Spclient depend on it.
Nothing internal depends on Dealer, so moving Connect() out of Session seems ok.

I see, I guess this is ok because the accesspoint is required to complete authentication so it has to be always started.

Seems to be fixed by setting the time that the pong can be behind to pingInterval + timeout. lastPong is now reset on reconnect to prevent a reconnect loop.

Sure, this makes sense.

@silenium-dev
silenium-dev requested a review from devgianlu August 20, 2024 16:17
@devgianlu
devgianlu merged commit 569ab07 into devgianlu:master Aug 20, 2024
@devgianlu

Copy link
Copy Markdown
Owner

Thanks!

devgianlu added a commit that referenced this pull request Aug 21, 2024
devgianlu added a commit that referenced this pull request Aug 22, 2024
@devgianlu

Copy link
Copy Markdown
Owner

Had to implement a couple of fixes, looks better now.

thilojaeggi pushed a commit to thilojaeggi/go-librespot that referenced this pull request Oct 13, 2024
* feat: start work on exposing Dealer.Connect()

* feat: implement dealer.Connect()

* feat: move running ping ticker and opening connection from NewAccesspoint() to Accesspoint.Connect()

* feat: move dealer.Connect() out of session, let caller handle it

* feat: move running accesspoint.pongAckTicker() to startReceiving()

* debug: add ping-pong trace logging for dealer and accesspoint

* feat: move running dealer.pingTicker() to dealer.startReceiving() and move initializing lastPong to startReceiving

* fix: reset dealer.lastPong on reconnect, increase pong tardiness interval to pingInterval+timeout

* fix: return immediately from Close() if connection not open

* fix: only close conn on receive loop termination when not stopping, remove read lock around recvLoop read
thilojaeggi pushed a commit to thilojaeggi/go-librespot that referenced this pull request Oct 13, 2024
thilojaeggi pushed a commit to thilojaeggi/go-librespot that referenced this pull request Oct 13, 2024
mcMineyC pushed a commit to mcMineyC/go-librespot_embedded that referenced this pull request Mar 27, 2026
* feat: start work on exposing Dealer.Connect()

* feat: implement dealer.Connect()

* feat: move running ping ticker and opening connection from NewAccesspoint() to Accesspoint.Connect()

* feat: move dealer.Connect() out of session, let caller handle it

* feat: move running accesspoint.pongAckTicker() to startReceiving()

* debug: add ping-pong trace logging for dealer and accesspoint

* feat: move running dealer.pingTicker() to dealer.startReceiving() and move initializing lastPong to startReceiving

* fix: reset dealer.lastPong on reconnect, increase pong tardiness interval to pingInterval+timeout

* fix: return immediately from Close() if connection not open

* fix: only close conn on receive loop termination when not stopping, remove read lock around recvLoop read
mcMineyC pushed a commit to mcMineyC/go-librespot_embedded that referenced this pull request Mar 27, 2026
mcMineyC pushed a commit to mcMineyC/go-librespot_embedded that referenced this pull request Mar 27, 2026
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.

When using as library, the dealer logs an error every 30 seconds about not having received a pong

2 participants