feat: add new features finally, race, timeout #7
Conversation
WalkthroughThe changes involve significant modifications to the promise handling functionality in the codebase. The Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Promise
participant Resolver
participant Rejector
Caller->>Promise: Create Promise
Promise->>Resolver: Start resolving
Promise->>Rejector: Handle rejection
Resolver-->>Promise: Resolve value
Rejector-->>Promise: Reject error
Promise-->>Caller: Return resolved value or error
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: 5
Outside diff range and nitpick comments (3)
promise_test.go (3)
365-404: Comprehensive testing of race conditions.The
TestPromise_Racefunction effectively tests both the fastest resolve and reject scenarios in race conditions. The use ofrequire.NoErrorandrequire.EqualinRaceWithFastestResolveensures that the correct promise wins the race, and similar assertions inRaceWithFastestRejectvalidate the error handling.Consider adding a brief comment above each sub-test to explain the setup and expected outcome for future maintainability.
407-439: Effective testing of finally block execution.The
TestPromise_Finallyfunction correctly tests the execution of thefinallyblock in both successful and error scenarios. The use ofrequire.NoErrorandrequire.TrueinFinallyAfterResolveconfirms that thefinallyblock executes after a successful resolution. Similarly,require.Errorandrequire.TrueinFinallyAfterRejectensure that thefinallyblock executes despite the promise rejection.Adding comments to explain the purpose of setting
finallyExecutedand its importance in the test could enhance readability and maintainability.
442-464: Accurate testing of timeout behavior.The
TestPromise_Timeoutfunction effectively tests the timeout behavior of promises. The sub-testTimeoutBeforeResolvecorrectly asserts an error when the promise resolution exceeds the specified timeout, usingrequire.Error. The sub-testResolveBeforeTimeoutconfirms that the promise resolves correctly within the timeout period, usingrequire.NoErrorandrequire.Equal.Consider adding more detailed comments to explain the setup and expected outcomes, especially the significance of the timeout values chosen for the tests.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- README.md (1 hunks)
- promise.go (4 hunks)
- promise_test.go (1 hunks)
Additional context used
Markdownlint
README.md
29-29: Column: 1
Hard tabs(MD010, no-hard-tabs)
30-30: Column: 1
Hard tabs(MD010, no-hard-tabs)
31-31: Column: 1
Hard tabs(MD010, no-hard-tabs)
32-32: Column: 1
Hard tabs(MD010, no-hard-tabs)
34-34: Column: 1
Hard tabs(MD010, no-hard-tabs)
35-35: Column: 1
Hard tabs(MD010, no-hard-tabs)
36-36: Column: 1
Hard tabs(MD010, no-hard-tabs)
37-37: Column: 1
Hard tabs(MD010, no-hard-tabs)
39-39: Column: 1
Hard tabs(MD010, no-hard-tabs)
Additional comments not posted (4)
README.md (3)
29-29: Question the removal ofctxfrom promise instantiation.The removal of the
contextparameter from the promise instantiation could significantly alter the behavior of these promises, particularly in scenarios involving cancellation or timeouts. Can you clarify the rationale behind this change? It's crucial to ensure that the promises still support these features if needed.Tools
Markdownlint
29-29: Column: 1
Hard tabs(MD010, no-hard-tabs)
34-34: Reiterate concerns regarding the removal ofctx.As with
p1, the removal ofcontextfromp2's instantiation needs clarification. This change could impact the promise's ability to handle cancellations and timeouts effectively.Tools
Markdownlint
34-34: Column: 1
Hard tabs(MD010, no-hard-tabs)
39-39: Repeat concerns forctxremoval inp3.The removal of
contextfrom the instantiation ofp3mirrors the concerns raised forp1andp2. It's important to understand the implications of this change on the promise's functionality, especially regarding cancellation and timeout handling.Tools
Markdownlint
39-39: Column: 1
Hard tabs(MD010, no-hard-tabs)
promise.go (1)
189-199: Ensure original promises handle context cancellation inTimeoutThe
Timeoutfunction relies on the original promise (p) respecting context cancellation to enforce the timeout. Ifpdoes not handle context cancellation, it may continue executing after the timeout, leading to potential resource leaks. Ensure that all promise tasks accept and correctly handle the context.Run the following script to check that all promise tasks accept a context parameter:
| return NewWithPool(func(resolve func(T), reject func(error)) { | ||
| result, err := p.Await(ctx) | ||
| fn() | ||
| if err != nil { | ||
| reject(err) | ||
| } else { | ||
| resolve(result) | ||
| } | ||
| }, defaultPool) |
There was a problem hiding this comment.
Ensure Finally function handles panics
In the Finally function, if fn() panics, it will propagate up and potentially crash the program. Consider wrapping the call to fn() in a recover block to handle any panics gracefully.
Apply this diff to handle panics in fn():
result, err := p.Await(ctx)
- fn()
+ func() {
+ defer func() {
+ if r := recover(); r != nil {
+ // Handle or log the panic as necessary
+ reject(fmt.Errorf("panic in Finally function: %v", r))
+ }
+ }()
+ fn()
+ }()
if err != nil {
reject(err)
} else {
resolve(result)
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return NewWithPool(func(resolve func(T), reject func(error)) { | |
| result, err := p.Await(ctx) | |
| fn() | |
| if err != nil { | |
| reject(err) | |
| } else { | |
| resolve(result) | |
| } | |
| }, defaultPool) | |
| return NewWithPool(func(resolve func(T), reject func(error)) { | |
| result, err := p.Await(ctx) | |
| func() { | |
| defer func() { | |
| if r := recover(); r != nil { | |
| // Handle or log the panic as necessary | |
| reject(fmt.Errorf("panic in Finally function: %v", r)) | |
| } | |
| }() | |
| fn() | |
| }() | |
| if err != nil { | |
| reject(err) | |
| } else { | |
| resolve(result) | |
| } | |
| }, defaultPool) |
| p.err.Store(err) | ||
| close(p.done) |
There was a problem hiding this comment.
Check for nil before storing errors in reject method
Storing a nil error in atomic.Value can cause panics during type assertions. Ensure that err is not nil before storing it, or handle nil errors appropriately.
Apply this diff to verify the error:
func (p *Promise[T]) reject(err error) {
p.once.Do(func() {
+ if err == nil {
+ err = fmt.Errorf("unknown error")
+ }
p.err.Store(err)
close(p.done)
})
}Committable suggestion was skipped due to low confidence.
| // Race returns a promise that resolves or rejects as soon as one of the promises resolves or rejects | ||
| func Race[T any](ctx context.Context, promises ...*Promise[T]) *Promise[T] { | ||
| return NewWithPool(func(resolve func(T), reject func(error)) { | ||
| for _, p := range promises { | ||
| go func(p *Promise[T]) { | ||
| result, err := p.Await(ctx) | ||
| if err != nil { | ||
| reject(err) | ||
| } else { | ||
| resolve(result) | ||
| } | ||
| }(p) | ||
| } | ||
| }, defaultPool) | ||
| } |
There was a problem hiding this comment.
Prevent goroutine leaks in Race function
In the Race function, once the promise is resolved or rejected, the remaining goroutines continue executing, which can lead to unnecessary resource usage. Consider canceling the context to stop other goroutines after the first promise settles.
Apply this diff to add context cancellation:
func Race[T any](ctx context.Context, promises ...*Promise[T]) *Promise[T] {
+ ctx, cancel := context.WithCancel(ctx)
return NewWithPool(func(resolve func(T), reject func(error)) {
+ defer cancel()
for _, p := range promises {
go func(p *Promise[T]) {
+ select {
+ case <-ctx.Done():
+ return
+ default:
+ }
result, err := p.Await(ctx)
if err != nil {
reject(err)
+ cancel()
} else {
resolve(result)
+ cancel()
}
}(p)
}
}, defaultPool)
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Race returns a promise that resolves or rejects as soon as one of the promises resolves or rejects | |
| func Race[T any](ctx context.Context, promises ...*Promise[T]) *Promise[T] { | |
| return NewWithPool(func(resolve func(T), reject func(error)) { | |
| for _, p := range promises { | |
| go func(p *Promise[T]) { | |
| result, err := p.Await(ctx) | |
| if err != nil { | |
| reject(err) | |
| } else { | |
| resolve(result) | |
| } | |
| }(p) | |
| } | |
| }, defaultPool) | |
| } | |
| // Race returns a promise that resolves or rejects as soon as one of the promises resolves or rejects | |
| func Race[T any](ctx context.Context, promises ...*Promise[T]) *Promise[T] { | |
| ctx, cancel := context.WithCancel(ctx) | |
| return NewWithPool(func(resolve func(T), reject func(error)) { | |
| defer cancel() | |
| for _, p := range promises { | |
| go func(p *Promise[T]) { | |
| select { | |
| case <-ctx.Done(): | |
| return | |
| default: | |
| } | |
| result, err := p.Await(ctx) | |
| if err != nil { | |
| reject(err) | |
| cancel() | |
| } else { | |
| resolve(result) | |
| cancel() | |
| } | |
| }(p) | |
| } | |
| }, defaultPool) | |
| } |
| var wg sync.WaitGroup | ||
| wg.Add(len(promises)) | ||
|
|
||
| for i, p := range promises { | ||
| i, p := i, p | ||
| pool.Go(func() { | ||
| defer wg.Done() | ||
| result, err := p.Await(ctx) | ||
| if err != nil { | ||
| reject(err) | ||
| return | ||
| } | ||
| results[i] = result | ||
| }) | ||
| } | ||
|
|
||
| wg.Wait() |
There was a problem hiding this comment.
Handle early rejection in AllWithPool
In AllWithPool, if one promise rejects, reject(err) is called, but other promises may continue executing, leading to unnecessary processing. Introduce context cancellation to stop processing remaining promises when one fails.
Apply this diff to implement cancellation:
func AllWithPool[T any](ctx context.Context, pool Pool, promises ...*Promise[T]) *Promise[[]T] {
if len(promises) == 0 {
panic("missing promises")
}
+ ctx, cancel := context.WithCancel(ctx)
+ defer cancel()
return NewWithPool(func(resolve func([]T), reject func(error)) {
results := make([]T, len(promises))
var wg sync.WaitGroup
wg.Add(len(promises))
for i, p := range promises {
i, p := i, p
pool.Go(func() {
defer wg.Done()
+ select {
+ case <-ctx.Done():
+ return
+ default:
+ }
result, err := p.Await(ctx)
if err != nil {
reject(err)
+ cancel()
return
}
results[i] = result
})
}
wg.Wait()
resolve(results)
}, pool)
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var wg sync.WaitGroup | |
| wg.Add(len(promises)) | |
| for i, p := range promises { | |
| i, p := i, p | |
| pool.Go(func() { | |
| defer wg.Done() | |
| result, err := p.Await(ctx) | |
| if err != nil { | |
| reject(err) | |
| return | |
| } | |
| results[i] = result | |
| }) | |
| } | |
| wg.Wait() | |
| ctx, cancel := context.WithCancel(ctx) | |
| defer cancel() | |
| var wg sync.WaitGroup | |
| wg.Add(len(promises)) | |
| for i, p := range promises { | |
| i, p := i, p | |
| pool.Go(func() { | |
| defer wg.Done() | |
| select { | |
| case <-ctx.Done(): | |
| return | |
| default: | |
| } | |
| result, err := p.Await(ctx) | |
| if err != nil { | |
| reject(err) | |
| cancel() | |
| return | |
| } | |
| results[i] = result | |
| }) | |
| } | |
| wg.Wait() |
| if err := p.err.Load(); err != nil { | ||
| var t T | ||
| return t, err.(error) | ||
| } | ||
| return p.value.Load().(T), nil |
There was a problem hiding this comment.
Safely handle type assertions from atomic.Value
When loading values from atomic.Value, type assertions can panic if the stored value is nil or of an unexpected type. To prevent potential panics, ensure to check for nil and handle type assertions safely using the comma-ok idiom.
Apply this diff to enhance safety:
case <-p.done:
if errInterface := p.err.Load(); errInterface != nil {
var t T
- return t, errInterface.(error)
+ if err, ok := errInterface.(error); ok {
+ return t, err
+ } else {
+ return t, fmt.Errorf("unexpected error type: %T", errInterface)
+ }
}
if valueInterface := p.value.Load(); valueInterface != nil {
- return p.value.Load().(T), nil
+ if value, ok := valueInterface.(T); ok {
+ return value, nil
+ } else {
+ var t T
+ return t, fmt.Errorf("unexpected value type: %T", valueInterface)
+ }
+ } else {
+ var t T
+ return t, fmt.Errorf("value is nil")
}Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
New Features
Race,Finally, andTimeout.Bug Fixes
Tests