fix: be sure that collector always initialized #130
Conversation
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
|
""" WalkthroughThe changes refactor the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Plugin
Caller->>Plugin: KvFromConfig()
Plugin->>Plugin: rwm.Lock()
Plugin->>Plugin: Set metricsCollector
Plugin->>Plugin: rwm.Unlock()
Caller->>Plugin: MetricsCollector()
Plugin->>Plugin: rwm.RLock()
Plugin->>Plugin: Check metricsCollector != nil
Plugin->>Plugin: rwm.RUnlock()
Plugin-->>Caller: Return metricsCollector or nil
Assessment against linked issues
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR addresses an issue where an uninitialized metrics collector was returned when the Redis driver is absent. Key changes include renaming the mutex field for clarity, adding an extra lock-check in KvFromConfig, and modifying the MetricsCollector method to guard against accessing an uninitialized collector.
Files not reviewed (1)
- go.work.sum: Language not supported
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugin.go (1)
73-76: Locking is fine, but the comment is misleadingThe write-lock is in fact required to avoid a data race when multiple goroutines may call
KvFromConfigconcurrently.
Consider revising the comment (// should not be needed, but to double check) to reflect the real necessity, otherwise future refactors might be tempted to remove what looks like defensive locking only.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
plugin.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Redis plugin (Go stable, PHP 8.4, OS ubuntu-latest)
🔇 Additional comments (1)
plugin.go (1)
34-34:✅ Verification successful
Renaming the anonymous
sync.RWMutexcan break external callers that locked the struct directlyExposing the mutex as an anonymous field previously allowed call-sites to do:
p.RLock()After renaming the field to
rwm, such code will no longer compile.
Inside this package you updated the references, but please verify that no other package takes a dependency on the anonymous lock.
🏁 Script executed:
#!/usr/bin/env bash # Search for direct calls to Plugin.RLock / Plugin.RUnlock outside redis/plugin.go rg --glob '!redis/plugin.go' -nE '\.RUnlock?\('Length of output: 446
Let’s rerun the search with the proper pattern to catch both
RLockandRUnlockcalls outside ofredis/plugin.go.
🏁 Script executed:
#!/usr/bin/env bash # Search for direct calls to RLock/RUnlock outside redis/plugin.go rg -n --glob '!redis/plugin.go' -e '\.RLock\(' -e '\.RUnlock\(' .Length of output: 363
🏁 Script executed:
#!/usr/bin/env bash # Locate plugin.go files and inspect the package declaration fd plugin.go sed -n '1,20p' plugin.goLength of output: 607
Let’s exclude
plugin.goitself and rerun the search:
🏁 Script executed:
#!/usr/bin/env bash # Search for direct calls to RLock/RUnlock outside plugin.go rg -n --glob '!plugin.go' -e '\.RLock\(' -e '\.RUnlock\(' .Length of output: 327
No external direct calls to
Plugin.RLock()orPlugin.RUnlock()foundOur search across the repository (excluding
plugin.go) did not locate any call‐sites using the old anonymoussync.RWMutexonPlugin. The onlyRLock/RUnlockinvocations live intests/mock/observer.goagainst a differentmufield, so renaming the embedded mutex torwmwon’t break external code.No further action required.
Reason for This PR
closes: roadrunner-server/roadrunner#2172
Description of Changes
metricsplugin whenMetricsCollectormethod was implemented. But, in our case, we returned not initialized collector when there're no Redis driver.License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit