Fritz!: cache session in Settings instead of per-API connection#29574
Merged
Conversation
Moves session caching (SID + last-fetch timestamp) into fritz.Settings. GetSessionID now returns the cached SID until the session expires and refreshes transparently. The aha and smarthome connections drop their own SID/updated fields and refreshSession helpers and just call GetSessionID where they need a token.
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Now that
Settingscachessid/updatedand is shared via pointers,GetSessionIDintroduces concurrent reads/writes to these fields across connections without synchronization; consider adding a mutex or other concurrency control around the cache to avoid data races. - Changing
GetSessionIDto use a pointer receiver and internal mutable state assumes each logical Fritzbox configuration uses a single shared*Settingsinstance; double-check that other call sites don’t copySettingsby value in a way that would unintentionally bypass the shared session cache.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `Settings` caches `sid`/`updated` and is shared via pointers, `GetSessionID` introduces concurrent reads/writes to these fields across connections without synchronization; consider adding a mutex or other concurrency control around the cache to avoid data races.
- Changing `GetSessionID` to use a pointer receiver and internal mutable state assumes each logical Fritzbox configuration uses a single shared `*Settings` instance; double-check that other call sites don’t copy `Settings` by value in a way that would unintentionally bypass the shared session cache.
## Individual Comments
### Comment 1
<location path="meter/fritz/types.go" line_range="24-25" />
<code_context>
URI, AIN, User, Password string
Firmware82 bool // use new REST API (FritzOS 8.2+)
+
+ sid string
+ updated time.Time
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Accessing the cached SID and updated time via Settings may introduce data races if Settings is shared across goroutines.
Previously these values were per-connection, so contention wasn’t an issue. Now they’re shared via `*Settings`, so concurrent `GetSessionID` calls on the same instance will read/write `sid` and `updated` without synchronization, leading to races. If `Settings` can be used from multiple goroutines, please add appropriate synchronization (e.g., a mutex) or otherwise make `GetSessionID` concurrency-safe.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refactor: pull session caching out of the AHA and Smarthome connections and let `fritz.Settings.GetSessionID` own it. Both API clients now just call the shared helper instead of maintaining their own `SID`/`updated` fields and `refreshSession` plumbing.
Test plan