fix: unify client shutdown around access point and dealer lifecycles#305
Conversation
|
I see your point and I like that it reduces the complexity overall. However, I am not fully sold on the fact that once a |
It is possible, but would be more complexity to support. The question is more if it's needed/wanted to keep IMO. Currently they are there, but not working as expected. If we want proper independent destruction, then we need to make it possible for them to unregister themselves with the AP. I don't mind adding that, if that is what you prefer. If so, I can do it in this PR or a follow-up. |
There was a problem hiding this comment.
Pull request overview
This PR unifies shutdown semantics across the access point (AP) and downstream consumers by making AP the single lifecycle owner and exposing an Accesspoint.Done() signal so components like mercury.Client and audio.KeyProvider can stop/return errors reliably when the AP is closed.
Changes:
- Replaces ad-hoc closed/stop flags with a single
donechannel in AP and Dealer, and updates loops to stop on<-done. - Removes
Close()frommercury.Clientandaudio.KeyProviderand makes their request/recv paths depend on AP shutdown (ap.Done()/ap.ErrAccesspointClosed). - Updates session shutdown and adds regression tests for shutdown signaling and post-close request failures.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
session/session.go |
Simplifies Session.Close() to rely on AP-owned shutdown. |
ap/ap.go |
Introduces done lifecycle + Done() accessor; updates connect/send/receive/loops to observe shutdown. |
ap/ap_test.go |
Updates ticker tests and adds coverage for Done() closing/idempotent Close. |
dealer/dealer.go |
Replaces stop/closed flags with done; updates loops and close/reconnect behavior. |
dealer/recv.go |
Updates receiver registration and request handling to respect dealer shutdown via done. |
dealer/dealer_test.go |
Adjusts ticker tests and closed-state behavior test to use done. |
mercury/client.go |
Removes client-local Close/stop; uses ap.Done() to stop and fail requests on AP close. |
mercury/client_test.go |
Adds regression test ensuring request fails with ErrAccesspointClosed after AP close. |
audio/provider.go |
Removes provider-local Close/stop; uses ap.Done() to stop and fail requests on AP close. |
audio/provider_test.go |
Adds regression test ensuring request fails with ErrAccesspointClosed after AP close. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Follow-up to #302 and #303. Those PRs hardened the lifecycle of ap and dealer themselves, but they. This PR is targeting a downstream issue from this, which is that mercury.Client and audio.KeyProvider have no way to observe when pa dies (which can cause some issues, as you might imagine).
The approach I took here is one I can adjust, but essentially the design does two things:
The main benefit of this design is that it simplifies the life cycle a lot (for instance, session close can now just rely on ap closing instead of having to deal with all the consumers too).
The downside is that it is an API breaking change: mercury.Client.Close() and audio.KeyProvider.Close() are removed.
If you want to consider alternative designs, I am happy to do that. I have gone through some iterations on this, and in the end, I thought this was the cleanest.
General summary of the changes (🤖):