cadence-client icon indicating copy to clipboard operation
cadence-client copied to clipboard

Ignore EOF errors in `jsonEncoding.Unmarshal()`

Open jjti opened this issue 2 years ago • 1 comments

What changed?

  • Right now, the default JSON decoder will try and fail to deserialize empty Workflow results to an *interface{} https://github.com/uber-go/cadence-client/issues/1016
    • mini re-production of what's happening: https://go.dev/play/p/Xhmnq5rA0By
  • This changes the deserialization behavior to ignore EOF errors, leaving the undecoded objs Zero-valued

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: https://github.com/uber-go/cadence-client/issues/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?

  • make test
  • added unit test
  • I've replicated the error of https://github.com/uber-go/cadence-client/issues/1016 in our local Replayer code. This change fixes them:
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

jjti avatar Sep 11 '22 14:09 jjti

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Sep 11 '22 14:09 CLAassistant

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

jjti avatar Apr 13 '23 01:04 jjti