-
Notifications
You must be signed in to change notification settings - Fork 41
garbage collection, global timeout #72
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
Conversation
… pool, fix a small race in a test, remove unused code
| // Validate input parameters | ||
| if input.rowsThreshold != nil && *input.rowsThreshold <= 0 { | ||
| return fmt.Errorf("rowsThreshold must be greater than 0, got %d", *input.rowsThreshold) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically this could be enforced by using a unsigned integer, but I trust this makes for a better error message.
| // Signal the child workflow is started | ||
| pollingHandleStartEvent.Set() | ||
|
|
||
| result, err := childHandle.GetResult() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get result from child workflow: %w", err) | ||
| } | ||
| return result, nil | ||
| case 2: | ||
| // Second handle will be a polling handle | ||
| _, ok := childHandle.(*workflowPollingHandle[string]) | ||
| if !ok { | ||
| return "", fmt.Errorf("expected recovered child handle to be of type workflowPollingHandle, got %T", childHandle) | ||
| } | ||
| } | ||
|
|
||
| // Signal the child workflow is started | ||
| pollingHandleStartEvent.Set() | ||
|
|
||
| result, err := childHandle.GetResult() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get result from child workflow: %w", err) | ||
| } | ||
| return result, nil | ||
| return "", nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an interesting race here.
This test calls the workflow twice, then eventually waits on the workflow result before completing. After completion, tests call a cleanup function that calls Shutdown.
It is possible that the 2nd invocation of the workflow (typically the one where the child gets a polling handle) completes slow enough that the test is already in the cleanup phase.
This will result in a segfault, given that Shutdown clears the system DB.
Because this test just wants to verify the 2nd invocation of the child gets a polling handle, we can simply do the GetResult() on the first call only.
| var blockingStepStopEvent *Event | ||
|
|
||
| func blockingStep(_ context.Context) (string, error) { | ||
| blockingStepStopEvent.Wait() | ||
| return "", nil | ||
| } | ||
|
|
||
| var idempotencyWorkflowWithStepEvent *Event | ||
|
|
||
| func idempotencyWorkflowWithStep(dbosCtx DBOSContext, input string) (int64, error) { | ||
| RunAsStep(dbosCtx, func(ctx context.Context) (int64, error) { | ||
| return incrementCounter(ctx, int64(1)) | ||
| }) | ||
| idempotencyWorkflowWithStepEvent.Set() | ||
| RunAsStep(dbosCtx, func(ctx context.Context) (string, error) { | ||
| return blockingStep(ctx) | ||
| }) | ||
| return idempotencyCounter, nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused
kraftp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! We'll also need to add Go to the Cloud GC tests
also: