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

Ignore EOF errors in jsonEncoding.Unmarshal() #1188

Closed

Conversation

jjti
Copy link

@jjti jjti commented Sep 11, 2022

What changed?

I expect this will better align with user expectations. For example, if you json.Unmarshal a 0-length JSON array into a 1-length slice of struct pointers, the pointer remains zero'ed without err: https://go.dev/play/p/ppz62H1Pqpw

To unmarshal a JSON array into a Go array, Unmarshal decodes JSON array elements into corresponding Go array elements. If the Go array is smaller than the JSON array, the additional JSON array elements are discarded. If the JSON array is smaller than the Go array, the additional Go array elements are set to zero values.

https://pkg.go.dev/encoding/json#Unmarshal:~:text=To%20unmarshal%20a%20JSON%20array%20into%20a%20Go%20array

Why?

I've found that the Cadence Replayer fails on many of my team's Workflows:

resp: RespondDecisionTaskCompletedRequest{
    TaskToken: [82 101 112 108 97 121 84 97 115 107 84 111 107 101 110],
    Decisions: [
        Decision{
            DecisionType: FailWorkflowExecution,
            FailWorkflowExecutionDecisionAttributes: FailWorkflowExecutionDecisionAttributes{
                Reason: cadenceInternal:Generic,
                Details: [117 110 97 98 108 101 32 116 111 32 100 101 99 111 100 101 32 97 114 103 117 109 101 110 116 58 32 48 44 32 42 105 110 116 101 114 102 97 99 101 32 123 125 44 32 119 105 116 104 32 106 115 111 110 32 101 114 114 111 114 58 32 69 79 70]}}],
                Identity: replayID,
                ReturnNewDecisionTask: true
                ForceCreateNewDecisionTask: false,
                BinaryChecksum: e5dd254c6d311f3fc3021e413368a456}

last: HistoryEvent{
    EventId: 386,
    Timestamp: 1662901315491153124,
    EventType: WorkflowExecutionCompleted,
    Version: -24,
    TaskId: 2831498722,
    WorkflowExecutionCompletedEventAttributes: WorkflowExecutionCompletedEventAttributes{DecisionTaskCompletedEventId: 385}}

Ie we're getting: unable to decode argument: 0, *interface {}, with json error: EOF in Cadence Replayer where the Workflows happily succeed in our production code. Others have noticed the same: #1016

This happens when we pass an interface{} to a future.Get(ctx, value) (as a throw away value, we don't actually expect a result):

	var result interface{}
	future := UpdateActivity.ExecuteRaw(ctx, p)
	return future.Get(ctx, &result)

How did you test it?

determinism_test.go:174: successfully replayed history

Potential risks

  • There's an implicit dependency somewhere on the EOF errors from Unmarshal in decoder.defaultDataConverter(data []byte, to ...interface{}). Eg: it's expected that we'll error out on Unmarshal and that all response values will be set to non-Zero values

@CLAassistant
Copy link

CLAassistant commented Sep 11, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ jjti
✅ ibarrajo
❌ jjtimmons


jjtimmons seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jjti jjti changed the title Fix jsonEncoding.Unmarshal to not err on empty `data Fix jsonEncoding.Unmarshal to not err on empty data Sep 11, 2022
@jjti jjti changed the title Fix jsonEncoding.Unmarshal to not err on empty data Fix jsonEncoding.Unmarshal() to not error on empty data Sep 11, 2022
@jjti jjti changed the title Fix jsonEncoding.Unmarshal() to not error on empty data Ignore EOF errors in jsonEncoding.Unmarshal() Sep 11, 2022
@jjti jjti marked this pull request as ready for review September 11, 2022 15:29
@jjti
Copy link
Author

jjti commented Apr 13, 2023

closing because I don't like seeing this sit open in my PRs page, still think ya'll should do it

@jjti jjti closed this Apr 13, 2023
@ibarrajo ibarrajo requested a review from a team November 6, 2024 23:14
@ibarrajo
Copy link

ibarrajo commented Nov 6, 2024

re-opening for review by the Cadence team

Copy link

codecov bot commented Nov 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.51%. Comparing base (fa329fd) to head (ba91135).

Additional details and impacted files
Files with missing lines Coverage Δ
internal/encoding.go 39.13% <100.00%> (+1.35%) ⬆️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa329fd...ba91135. Read the comment docs.

@ibarrajo
Copy link

ibarrajo commented Nov 6, 2024

@jjti could you review the Contributor License Agreement?

Thanks!

@ibarrajo ibarrajo closed this Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants