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

Replicate official SDK to fetch results from Temporal #305

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

abhishekjain16
Copy link

While running long workflows that require synchronous results, await_workflow_result throws the following exception:

await_workflow_result': undefined method 'type' for nil:NilClass (NoMethodError)

pointing to line https://github.com/coinbase/temporal-ruby/blob/master/lib/temporal/client.rb#L251

Reason:
When trying to fetch workflow execution history with event_type: :close, Temporal sends empty events payload after next event (not necessarily close event)
Calling .first.type fails on empty event.

Fix:
Follow what official SDK does in Go and TS. Create a loop and try and fetch close event till it can find one
TS Example:
https://github.com/temporalio/sdk-typescript/blob/913e67e6381c421c3ed02ee24518adfa1243c84c/packages/client/src/workflow-client.ts#L630

Go Example:
https://github.com/temporalio/sdk-go/blob/626785769094562b39662d6076304c807b596ed9/internal/internal_workflow_client.go#L433-L436

This PR tries to replicate the same behavior

lib/temporal/client.rb Outdated Show resolved Hide resolved
lib/temporal/client.rb Outdated Show resolved Hide resolved
lib/temporal/client.rb Show resolved Hide resolved
@morgan121
Copy link

morgan121 commented Sep 22, 2024

@bdchauvette We are also getting this error now, is it possible to merge this fix? It's causing us a bit of headache

@abhishekjain16
Copy link
Author

abhishekjain16 commented Oct 24, 2024

@cj-cb @DeRauk Do you think we can merge this? I can fix conflict but only if there is a need to do it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants