Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "fix Compile clear() method bug and [pick to 1.2] fix a bug that process's cancel not match to its context " #17362

Merged
merged 2 commits into from
Jul 7, 2024

Conversation

aressu1985
Copy link
Contributor

@aressu1985 aressu1985 commented Jul 7, 2024

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #17358 https://github.com/matrixorigin/MO-Cloud/issues/3692 #17350

What this PR does / why we need it:

These two pr will cause :
1、the sysbench test hung
2、lots of orphan transaction during stability test on distributed mode
3、mo hung during stability test on distributed mode


PR Type

Bug fix


Description

  • Reverted changes to the reset method in the Compile class to avoid setting proc.Ctx to nil.
  • Reverted changes to the newCompile method in the messageReceiverOnServer class to restore proc.Ctx assignment.
  • Reverted context handling changes in the New method of the Process class to use the original context handling.

Changes walkthrough 📝

Relevant files
Bug fix
compile.go
Revert changes to `reset` method in `Compile` class           

pkg/sql/compile/compile.go

  • Reverted setting proc.Ctx to nil in reset method.
+1/-0     
remoterunServer.go
Revert changes to newCompile method in messageReceiverOnServer class

pkg/sql/compile/remoterunServer.go

  • Reverted removal of proc.Ctx assignment in newCompile method.
+0/-1     
process.go
Revert context handling changes in `New` method of `Process` class

pkg/vm/process/process.go

  • Reverted changes to context handling in New method.
+2/-7     

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

Copy link

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review

Context Management:
The changes in context handling in pkg/vm/process/process.go and pkg/sql/compile/remoterunServer.go revert to previous behavior. Ensure that this reversion does not reintroduce previously fixed issues related to context management.

Resource Cleanup:
The removal of the cancel context in pkg/vm/process/process.go may affect how resources are cleaned up. Verify that all resources are properly released when a process is terminated.

@mergify mergify bot requested a review from sukki37 July 7, 2024 13:57
@mergify mergify bot added the kind/bug Something isn't working label Jul 7, 2024
@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Jul 7, 2024
Copy link

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible issue
Ensure ongoing operations using the context are canceled before setting the context to nil

Before setting c.proc.Ctx to nil, ensure that any ongoing operations using this context
are properly canceled to avoid potential resource leaks or undefined behavior.

pkg/sql/compile/compile.go [169]

+if c.proc.Ctx != nil {
+    c.proc.Cancel()
+}
 c.proc.Ctx = nil
 
  • Apply this suggestion
Suggestion importance[1-10]: 8

Why: This suggestion is crucial as it addresses potential resource leaks or undefined behavior by ensuring that operations using the context are properly canceled before setting it to nil.

8
Ensure the context is not nil before initializing IncrService

Ensure that ctx is not nil before using it to initialize IncrService to avoid potential
runtime errors.

pkg/vm/process/process.go [69]

-IncrService:  incrservice.GetAutoIncrementService(ctx),
+IncrService:  incrservice.GetAutoIncrementService(nonNilContext(ctx)),
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why: This suggestion is relevant as it prevents potential runtime errors by ensuring that the context used to initialize IncrService is not nil.

7
Possible bug
Add a nil check for runningCancel before assignment to prevent nil pointer dereference

Add a check to ensure runningCancel is not nil before assigning it to proc.Cancel to
prevent potential nil pointer dereference.

pkg/sql/compile/remoterunServer.go [390]

-proc.Cancel = runningCancel
+if runningCancel != nil {
+    proc.Cancel = runningCancel
+}
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why: The suggestion correctly identifies a potential nil pointer dereference issue and proposes a preventive check, which is important for robust error handling.

7
Ensure the context is not nil before assigning it to Ctx

Add a check to ensure ctx is not nil before assigning it to Ctx to prevent potential nil
pointer dereference.

pkg/vm/process/process.go [65]

-Ctx:          ctx,
+Ctx:          nonNilContext(ctx),
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why: The suggestion correctly addresses a common issue of nil pointer dereference by ensuring the context is validated before use, which is crucial for stable code execution.

7

@mergify mergify bot merged commit 01fa7b1 into matrixorigin:1.2-dev Jul 7, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug fix kind/bug Something isn't working Review effort [1-5]: 2 size/S Denotes a PR that changes [10,99] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants