Add a MaxVariables limits #6
Conversation
|
Warning Rate limit exceeded@bdeneux has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 25 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 introduce a more structured approach to variable management within the codebase, shifting from simple integer counters to encapsulated objects that maintain state. This enhances thread safety, improves test reliability by reducing global state usage, and enforces constraints on variable creation. New tests have been added to validate the system's behavior under maximum variable limits, further strengthening robustness and maintainability. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VM
participant VariableManager
User->>VM: SetMaxVariables(10)
VM->>VariableManager: Update maxVariables
VariableManager-->>VM: Acknowledge update
VM->>User: Confirmation of max variable limit set
User->>VM: ResetEnv()
VM->>VariableManager: Reset with current maxVariables
VariableManager-->>VM: Environment reset
VM->>User: Confirmation of environment reset
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## main #6 +/- ##
=======================================
Coverage 98.33% 98.34%
=======================================
Files 23 23
Lines 6503 6513 +10
=======================================
+ Hits 6395 6405 +10
Misses 79 79
Partials 29 29
|
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Commits
Files that changed from the base of the PR and between ff37990 and e8d9489d0da6fd8d4e74e17aa32770ab5f523f8e.
Files selected for processing (6)
- engine/builtin_test.go (1 hunks)
- engine/text_test.go (2 hunks)
- engine/variable.go (1 hunks)
- engine/variable_test.go (1 hunks)
- engine/vm.go (3 hunks)
- engine/vm_test.go (3 hunks)
Additional comments not posted (15)
engine/variable.go (6)
10-10: LGTM!The introduction of
errMaxVariableswith a descriptive error message is appropriate.
12-12: LGTM!The introduction of
maxVariableswith a default value of 0 (indicating no limit) is sensible and aligns with the existing design.
13-18: LGTM!The replacement of the atomic counter with a mutex-protected structure for
varCounterenhances thread safety and prevents potential race conditions.
21-23: LGTM!The use of
LockandUnlockinlastVariableensures thread-safe access to the variable count.
31-37: LGTM!The addition of the limit check and panic mechanism in
NewVariableis a robust way to enforce resource constraints. The use ofLockandUnlockensures thread-safe access to the variable count.
Line range hint
144-179:
LGTM!The test function
Test_maxVariablescovers various scenarios and usesassert.Panicsandassert.Equalto accurately validate the expected behavior.engine/variable_test.go (1)
144-179: LGTM!The test function
Test_maxVariablescovers various scenarios and usesassert.Panicsandassert.Equalto accurately validate the expected behavior.engine/vm_test.go (2)
273-280: LGTM!The test function
TestVM_SetMaxVariablesverifies the behavior of theSetMaxVariablesmethod and usesassert.Equalto accurately validate the expected behavior.
Line range hint
301-321:
LGTM!The modifications to
TestVM_ResetEnvensure that the test accurately validates the count of variables and themaxVariablesvalue after the environment reset. The use ofassert.Equalensures that the test accurately validates the expected behavior.engine/vm.go (3)
73-75: LGTM! The addition of themaxVariablesfield is straightforward.The new field aligns with the PR objectives to manage the maximum number of variables in the VM.
282-288: LGTM! Consider adding a check for non-negative values.The
SetMaxVariablesmethod is clear and aligns with the PR objectives. However, it might be beneficial to include a check to ensure that the value is non-negative.func (vm *VM) SetMaxVariables(n uint64) { + if n < 0 { + panic("maxVariables cannot be negative") + } vm.maxVariables = n maxVariables = n }
Line range hint
291-300: LGTM! The modification toResetEnvis straightforward.The changes align with the PR objectives to manage the maximum number of variables in the VM.
engine/text_test.go (2)
27-27: LGTM! The modification tovarCounterinitialization is straightforward.The change aligns with the PR objectives to enhance encapsulation and clarity.
486-486: LGTM! The modification tovarCounterin the test loop is straightforward.The change aligns with the PR objectives to enhance encapsulation and clarity.
engine/builtin_test.go (1)
955-955: Verify the initialization ofvarCounter.count.The change from
varCounter = 1tovarCounter.count = 1suggests thatvarCounteris now a struct. Ensure that this initialization is correct and aligns with the new struct definition. Verify that all usages ofvarCounterin the test suite are updated accordingly.Verification successful
Initialization of
varCounter.countis correct and consistent.The change from
varCounter = 1tovarCounter.count = 1aligns with the struct definition ofvarCounter. The usage ofvarCounter.countis consistent across the codebase, confirming the correctness of the initialization.
engine/variable.go: Definition and usage ofvarCounter.count.engine/vm_test.go: Usage ofvarCounter.count.engine/text_test.go: Usage ofvarCounter.count.engine/vm.go: Usage ofvarCounter.count.engine/variable_test.go: Usage ofvarCounter.count.engine/builtin_test.go: Initialization ofvarCounter.count.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization and usage of `varCounter` in the test suite. # Test: Search for the definition and usage of `varCounter`. Expect: Correct struct definition and consistent usage. rg --type go $'varCounter'Length of output: 1066
bdeneux
force-pushed
the
feat/handle-max-variables
branch
from
e8d9489 to
0358f8f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
engine/vm_test.go (1)
Line range hint
301-321:
Consider adding comments to explain assertions.Adding comments to explain the purpose of each assertion can improve readability and maintainability of the test.
vm.SetMaxVariables(20) // Set max variables to 20 varCounter.count = 10 // Simulate existing variable count varContext = NewVariable() // Create a new variable context rootContext = NewAtom("non-root") // Set root context to a non-root atom rootEnv = &Env{ binding: binding{ key: newEnvKey(varContext), value: NewAtom("non-root"), }, } maxVariables = 30 // Set max variables to 30 t.Run("Reset environment", func(t *testing.T) { vm.ResetEnv() assert.Equal(t, uint64(1), varCounter.count) // Verify variable count is reset to 1 assert.Equal(t, "root", rootContext.String()) // Verify root context is reset assert.Equal(t, newEnvKey(varContext), rootEnv.binding.key) // Verify root environment key is reset assert.Equal(t, NewAtom("root"), rootEnv.binding.value) // Verify root environment value is reset assert.Equal(t, uint64(20), maxVariables) // Verify max variables is reset to 20 })
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Commits
Files that changed from the base of the PR and between e8d9489d0da6fd8d4e74e17aa32770ab5f523f8e and 0358f8f.
Files selected for processing (3)
- engine/variable_test.go (1 hunks)
- engine/vm.go (3 hunks)
- engine/vm_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- engine/variable_test.go
- engine/vm.go
📝 Description
This PR introduces a new global variable to define the maximum number of variables that can be created in the interpreter. This is used to prevent memory overflow or latency issues in predicates that loop on
NewVariable()calls.Due to the current design of
NewVariable(), which uses a global variable counter, I also had to use a global variable to handle themaxVariableslimit. To avoid changing the signature ofNewVariable(), which is used extensively throughout the interpreter, the only feasible way to throw an error when the limit is reached is to trigger a panic. This panic is caught by the interpreter and returns a Prolog error without stopping the interpreter.By default, no limit is set (0 means no limit).
If you have any idea to do in a better way 😇
Used to fix : axone-protocol/axoned#618