-
Notifications
You must be signed in to change notification settings - Fork 38
Cancellable get results #169
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
dbos/workflow.go
Outdated
| case <-h.dbosContext.Done(): | ||
| return *new(R), h.dbosContext.Err() |
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.
@maxdml I'm unable to get to this code path, so not sure if we really need it.
Tried shutting down DBOS server, but it get DB error from h.outcomeChan instead.
Let me know your thoughts
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.
For this path to kick in, you need to manually Launch and Shutdown the DBOSContext in a test. The sequence would be:
- Launch
- Run blocking workflow (so that the function doesn't return and its result never reaches
outcomeChan) - GetResult with no timeout
- Shutdown DBOSContext
- GetResult should result the error (which should be a context.Context DeadlineExceeded error)
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.
For this to work, the workflow must signal that it started, which you can achieve by using the test utilities "event" API. See an example here.
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.
For this case, should it be context.Canceled instead?
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.
You mean for a test exercising case <-h.dbosContext.Done(): ? You're right, it depends on how the context is cancelled (manually or through a deadline)
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.
Thanks @PhakornKiong . Left a few comments. Let me know if you have more questions!
dbos/workflow.go
Outdated
| case <-h.dbosContext.Done(): | ||
| return *new(R), h.dbosContext.Err() |
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.
For this path to kick in, you need to manually Launch and Shutdown the DBOSContext in a test. The sequence would be:
- Launch
- Run blocking workflow (so that the function doesn't return and its result never reaches
outcomeChan) - GetResult with no timeout
- Shutdown DBOSContext
- GetResult should result the error (which should be a context.Context DeadlineExceeded error)
dbos/workflow.go
Outdated
| case <-h.dbosContext.Done(): | ||
| return *new(R), h.dbosContext.Err() |
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.
For this to work, the workflow must signal that it started, which you can achieve by using the test utilities "event" API. See an example here.
8653f7d to
523e6af
Compare
523e6af to
57a0a17
Compare
|
@maxdml Noticed that there are test like I tired running on local as well as on my forked repo and it went fine. Let me know if there are any other changes that i should make |
57a0a17 to
9217f7b
Compare
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.
Thanks @PhakornKiong
Resolves #85