fix: Always initialize tracer #136
Conversation
Signed-off-by: Valery Piashchynski <[email protected]>
|
Warning Rate limit exceeded@rustatian has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request enhance the tracing capabilities of the Changes
Assessment against linked issues
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
tests/jobs_general_test.go (2)
150-221: Consider adding explicit verification of initialization orderWhile the test sets up the environment correctly, it could be enhanced to explicitly verify that the Jobs API remains stable when accessed before full plugin initialization.
Consider adding these verifications:
- Add assertions to verify the tracer initialization
- Add a test case that attempts to access the Jobs API immediately after startup
- Verify no segfault occurs during rapid initialization/shutdown cycles
func TestIssue2085(t *testing.T) { // ... existing setup ... time.Sleep(time.Second * 5) + + // Verify immediate Jobs API access doesn't cause segfault + t.Run("ImmediateAccess", helpers.PushToPipe("test-1-memory", false, "127.0.0.1:6001", []byte("test"))) + + // Verify tracer initialization + assert.NotNil(t, cont.Get("jobs")) stopCh <- struct{}{} wg.Wait()
215-215: Consider reducing sleep durationThe 5-second sleep might be longer than necessary for initialization.
- time.Sleep(time.Second * 5) + time.Sleep(time.Second * 2)plugin.go (2)
Line range hint
141-144: Consider consolidating tracer initializationWhile the defensive nil check is good, having initialization in both
InitandServesuggests we might need to consolidate our initialization strategy. Consider:
- Moving all tracer initialization logic to
Init- Adding validation in
Initto ensure tracer is properly initialized- if p.tracer == nil { - // noop tracer - p.tracer = sdktrace.NewTracerProvider() - }
Line range hint
724-773: Critical: Fix potential resource leak in command handlingThe error handling in the command processing could lead to resource leaks and has an incorrect Destroy call:
- The span.End() calls are scattered and might be missed in error paths
- There's an unconditional Destroy call after the switch statement which could double-free resources
Consider refactoring to use defer for span.End() and removing the incorrect Destroy call:
func (p *Plugin) readCommands() { for { select { case ev := <-p.eventsCh: ctx, span := p.tracer.Tracer(spanName).Start(context.Background(), "read_command", trace.WithSpanKind(trace.SpanKindServer)) + defer span.End() p.log.Debug("received JOBS event", zap.String("message", ev.Message()), zap.String("pipeline", ev.Plugin())) // message can be 'restart', 'stop'. switch ev.Message() { case stopStr: pipeline := ev.Plugin() _, _, err := p.pipelineExists(pipeline) if err != nil { p.log.Warn("failed to restart the pipeline", zap.Error(err), zap.String("pipeline", pipeline)) - span.End() continue } err = p.Destroy(context.Background(), pipeline) if err != nil { p.log.Error("failed to stop the pipeline", zap.Error(err), zap.String("pipeline", pipeline)) } p.log.Info("pipeline was stopped", zap.String("pipeline", pipeline)) case restartSrt: // ... rest of the switch } - err := p.Destroy(ctx, ev.Plugin()) - if err != nil { - p.log.Error("failed to destroy the pipeline", zap.Error(err), zap.String("pipeline", ev.Plugin())) - } - span.End() case <-p.stopCh: return } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
plugin.go(1 hunks)tests/configs/.rr-issue2085.yaml(1 hunks)tests/jobs_general_test.go(1 hunks)tests/php_test_files/jobs/jobs_ok.php(1 hunks)tests/php_test_files/jobs/on-init.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/configs/.rr-issue2085.yaml
🔇 Additional comments (5)
tests/php_test_files/jobs/jobs_ok.php (1)
13-17: LGTM: Task acknowledgment logic is correct
The error handling is properly maintained while updating the task completion method from complete() to ack().
tests/php_test_files/jobs/on-init.php (2)
15-19: LGTM: Jobs initialization is properly configured
The Jobs instance is correctly initialized with RPC connection using environment-provided address.
21-21: Verify if count() is the most appropriate method for initialization
While count() works to trigger initialization, consider if there's a more explicit method for this purpose.
✅ Verification successful
Using count() is appropriate in this initialization context
The code is a test file that verifies the basic initialization of the Jobs system. The count() method is a simple, lightweight way to verify that the Jobs instance is properly connected and functional. Since this is a test file specifically for initialization (on-init.php), using count() is appropriate as it:
- Provides a basic connectivity check
- Doesn't modify any state
- Is a minimal operation to verify the Jobs system is responsive
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for alternative initialization methods in the Jobs class
ast-grep --pattern 'class Jobs {
$$$
init($$$) {
$$$
}
$$$
}'
Length of output: 75
Script:
#!/bin/bash
# Let's search for the Jobs class implementation and its methods
rg -A 10 "class Jobs"
# Also search for any usage of count() method
rg "->count\(\)" -A 3
Length of output: 452
Script:
#!/bin/bash
# Let's search for the Jobs class implementation and its methods
rg "class Jobs"
# Also search for any usage of count method with proper escaping
rg "count\(\)"
# Let's also look at the specific test file content
cat "tests/php_test_files/jobs/on-init.php"
Length of output: 476
plugin.go (2)
131-131: LGTM: Defensive tracer initialization added
This change ensures the tracer is always initialized, preventing potential nil pointer dereferences. This directly addresses the segmentation fault issue mentioned in #2085.
Line range hint 1-1: Verify tracer usage across the codebase
Let's verify that all tracer usages have been properly updated to handle the new initialization pattern.
✅ Verification successful
All tracer usages are properly initialized and consistent
Based on the search results, all tracer usages follow a consistent pattern where p.tracer.Tracer() is called to create spans across different operations. There are no nil checks found, indicating that the tracer is properly initialized before use. The tracer is used consistently in:
- Command handling (plugin.go)
- RPC operations (rpc.go)
- Event listening (listener.go)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all tracer usages to ensure proper initialization
rg -A 2 "p\.tracer\."
# Search for potential nil checks that might indicate missed initialization
ast-grep --pattern 'if $x.tracer == nil { $$$ }'
Length of output: 2030
…ypes Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
pipeline.go (1)
Line range hint
1-1: Changes don't align with PR objectivesThe modifications in this file focus on integer parsing behavior, which appears unrelated to the PR's objective of fixing segmentation faults during Jobs API initialization. The changes here could introduce new issues without addressing the core problem described in issue #2085.
Consider:
- Focusing on the actual initialization issue in the Jobs API
- Removing these unrelated changes to minimize risk
- Creating a separate PR if integer parsing changes are needed
🧰 Tools
🪛 golangci-lint (1.62.2)
5-5: File is not
goimports-ed(goimports)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
pipeline.go(2 hunks)
🔇 Additional comments (2)
pipeline.go (2)
6-6: LGTM: Import addition is appropriate
The addition of the "math" package import is necessary for the integer bounds checking implementation.
94-104:
Critical: Potentially breaking changes in integer parsing
The changes to the Int method raise several concerns:
- Changing from 64-bit to 32-bit parsing is a potentially breaking change for existing clients that might be using large integers.
- The bounds check is redundant since
strconv.ParseIntwith size 32 already handles this internally. - This change appears unrelated to the PR's objective of fixing segmentation faults in the Jobs API initialization.
Suggested fix:
- res, err := strconv.ParseInt(v, 10, 32)
+ res, err := strconv.ParseInt(v, 10, 64)
- if res > math.MaxInt32 || res < math.MinInt32 {
- // return default if out of bounds
- return d
- }Let's verify if there are any existing uses of large integers that might be affected:
Signed-off-by: Valery Piashchynski <[email protected]>
Reason for This PR
closes: roadrunner-server/roadrunner#2085
Description of Changes
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
Release Notes
New Features
Bug Fixes
Tests