fix 0.18.4

Makefile

@@ -10,7 +10,7 @@ IMPORT_ROOT := go.uber.org/cadence
 THRIFT_GENDIR := .gen/go
 THRIFTRW_SRC := idls/thrift/cadence.thrift idls/thrift/shadower.thrift
 # one or more thriftrw-generated file(s), to create / depend on generated code
-THRIFTRW_OUT := $(THRIFT_GENDIR)/cadence/idl.go
+THRIFTRW_OUT := $(THRIFT_GENDIR)/cadence/cadence.go
 TEST_ARG ?= -v -race
 
 # general build-product folder, cleaned as part of `make clean`
@@ -134,11 +134,13 @@ bins: thriftc $(ALL_SRC) $(BUILD)/copyright lint $(BUILD)/dummy
 unit_test: $(BUILD)/dummy
 	@mkdir -p $(COVER_ROOT)
 	@echo "mode: atomic" > $(UT_COVER_FILE)
-	@for dir in $(UT_DIRS); do \
+	@failed=0; \
+	for dir in $(UT_DIRS); do \
 		mkdir -p $(COVER_ROOT)/"$$dir"; \
-		go test "$$dir" $(TEST_ARG) -coverprofile=$(COVER_ROOT)/"$$dir"/cover.out || exit 1; \
+		go test "$$dir" $(TEST_ARG) -coverprofile=$(COVER_ROOT)/"$$dir"/cover.out || failed=1; \
 		cat $(COVER_ROOT)/"$$dir"/cover.out | grep -v "mode: atomic" >> $(UT_COVER_FILE); \
-	done;
+	done; \
+	exit $$failed
 
 integ_test_sticky_off: $(BUILD)/dummy
 	@mkdir -p $(COVER_ROOT)

activity/doc.go

@@ -65,9 +65,14 @@ parameter is the standard Go context.
 The second string parameter is a custom activity-specific parameter that can be used to pass in data into the activity
 on start. An activity can have one or more such parameters. All parameters to an activity function must be
 serializable, which essentially means that params can’t be channels, functions, variadic, or unsafe pointer.
+Exact details will depend on your DataConverter, but by default they must work with encoding/json.Marshal (and
+Unmarshal on the receiving side, which has the same limitations plus generally cannot deserialize into an interface).
 
-The activity declares two return values: (string, error). The string return value is used to return the result of the
-activity. The error return value is used to indicate an error was encountered during execution.
+This activity declares two return values: (string, error). The string return value is used to return the result of the
+activity, and can be retrieved in the workflow with this activity's Future.
+The error return value is used to indicate an error was encountered during execution.
+Results must be serializable, like parameters, but only a single result value is allowed (i.e. you cannot return
+(string, string, error)).
 
 Implementation
 
@@ -77,8 +82,9 @@ constructs.
 
 Failing the activity
 
-To mark an activity as failed, all that needs to happen is for the activity function to return an error via the error
-return value.
+To mark an activity as failed, return an error from your activity function via the error return value.
+Note that failed activities do not record the non-error return's value: you cannot usefully return both a
+value and an error, only the error will be recorded.
 
 Activity Heartbeating
 
@@ -112,10 +118,13 @@ payload containing progress information.
 Activity Cancellation
 
 When an activity is cancelled (or its workflow execution is completed or failed) the context passed into its function
-is cancelled which sets its Done channel’s closed state. So an activity can use that to perform any necessary cleanup
-and abort its execution. Currently cancellation is delivered only to activities that call RecordHeartbeat.
+is cancelled which closes its Done() channel. So an activity can use that to perform any necessary cleanup
+and abort its execution.
 
-Async/Manual Activity Completion
+Currently, cancellation is delivered only to activities that call RecordHeartbeat.  If heartbeating is not performed,
+the activity will continue to run normally, but fail to record its result when it completes.
+
+Async and Manual Activity Completion
 
 In certain scenarios completing an activity upon completion of its function is not possible or desirable.
 
@@ -178,7 +187,7 @@ For a full example of implementing this pattern see the Expense sample.
 
 Registration
 
-In order to for some workflow execution to be able to invoke an activity type, the worker process needs to be aware of
+In order for a workflow to be able to execute an activity type, the worker process needs to be aware of
 all the implementations it has access to. An activity is registered with the following call:
 
     activity.Register(SimpleActivity)

client/client.go

@@ -25,7 +25,9 @@ package client
 
 import (
 	"context"
+	"errors"
 
+	"go.uber.org/cadence"
 	"go.uber.org/cadence/.gen/go/cadence/workflowserviceclient"
 	s "go.uber.org/cadence/.gen/go/shared"
 	"go.uber.org/cadence/encoded"
@@ -450,3 +452,54 @@ func NewValue(data []byte) encoded.Value {
 func NewValues(data []byte) encoded.Values {
 	return internal.NewValues(data)
 }
+
+// IsWorkflowError returns true if an error is a known returned-by-the-workflow error type, when it comes from
+// WorkflowRun from either Client.ExecuteWorkflow or Client.GetWorkflow.  If it returns false, the error comes from
+// some other source, e.g. RPC request failures or bad arguments.
+// Using it on errors from any other source is currently undefined.
+//
+// This checks for known types via errors.As, so it will check recursively if it encounters wrapped errors.
+// Note that this is different than the various `cadence.Is[Type]Error` checks, which do not unwrap.
+//
+// Currently the complete list of errors checked is:
+// *cadence.CustomError, *cadence.CanceledError, *workflow.ContinueAsNewError,
+// *workflow.GenericError, *workflow.TimeoutError, *workflow.TerminatedError,
+// *workflow.PanicError, *workflow.UnknownExternalWorkflowExecutionError
+//
+// See documentation for each error type for details.
+func IsWorkflowError(err error) bool {
+	var custom *cadence.CustomError
+	if errors.As(err, &custom) {
+		return true
+	}
+	var cancel *cadence.CanceledError
+	if errors.As(err, &cancel) {
+		return true
+	}
+
+	var generic *workflow.GenericError
+	if errors.As(err, &generic) {
+		return true
+	}
+	var timeout *workflow.TimeoutError
+	if errors.As(err, &timeout) {
+		return true
+	}
+	var terminate *workflow.TerminatedError
+	if errors.As(err, &terminate) {
+		return true
+	}
+	var panicked *workflow.PanicError
+	if errors.As(err, &panicked) {
+		return true
+	}
+	var can *workflow.ContinueAsNewError
+	if errors.As(err, &can) {
+		return true
+	}
+	var unknown *workflow.UnknownExternalWorkflowExecutionError
+	if errors.As(err, &unknown) {
+		return true
+	}
+	return false
+}

compatibility/thrift2proto.go

@@ -36,4 +36,4 @@ func NewThrift2ProtoAdapter(
 	visibility apiv1.VisibilityAPIYARPCClient,
 ) workflowserviceclient.Interface {
 	return internal.NewThrift2ProtoAdapter(domain, workflow, worker, visibility)
-}
+}

evictiontest/workflow_cache_eviction_test.go

@@ -44,9 +44,22 @@ import (
 	"go.uber.org/cadence/internal/common"
 	"go.uber.org/cadence/worker"
 	"go.uber.org/yarpc"
+	"go.uber.org/zap/zaptest"
 	"golang.org/x/net/context"
 )
 
+// copied from internal/test_helpers_test.go
+// this is the mock for yarpcCallOptions, as gomock requires the num of arguments to be the same.
+// see getYarpcCallOptions for the default case.
+func callOptions() []interface{} {
+	return []interface{}{
+		gomock.Any(), // library version
+		gomock.Any(), // feature version
+		gomock.Any(), // client name
+		gomock.Any(), // feature flags
+	}
+}
+
 func testReplayWorkflow(ctx internal.Context) error {
 	ao := internal.ActivityOptions{
 		ScheduleToStartTimeout: time.Second,
@@ -86,9 +99,6 @@ func TestWorkersTestSuite(t *testing.T) {
 	suite.Run(t, new(CacheEvictionSuite))
 }
 
-// this is the mock for yarpcCallOptions, make sure length are the same
-var callOptions = []interface{}{gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()}
-
 func createTestEventWorkflowExecutionStarted(eventID int64, attr *m.WorkflowExecutionStartedEventAttributes) *m.HistoryEvent {
 	return &m.HistoryEvent{
 		EventId:                                 common.Int64Ptr(eventID),
@@ -149,25 +159,28 @@ func (s *CacheEvictionSuite) TestResetStickyOnEviction() {
 	cacheSize := 5
 	internal.SetStickyWorkflowCacheSize(cacheSize)
 	// once for workflow worker because we disable activity worker
-	s.service.EXPECT().DescribeDomain(gomock.Any(), gomock.Any(), callOptions...).Return(nil, nil).Times(1)
+	s.service.EXPECT().DescribeDomain(gomock.Any(), gomock.Any(), callOptions()...).Return(nil, nil).Times(1)
 	// feed our worker exactly *cacheSize* "legit" decision tasks
 	// these are handcrafted decision tasks that are not blatantly obviously mocks
 	// the goal is to trick our worker into thinking they are real so it
 	// actually goes along with processing these and puts their execution in the cache.
-	s.service.EXPECT().PollForDecisionTask(gomock.Any(), gomock.Any(), callOptions...).DoAndReturn(mockPollForDecisionTask).Times(cacheSize)
+	s.service.EXPECT().PollForDecisionTask(gomock.Any(), gomock.Any(), callOptions()...).DoAndReturn(mockPollForDecisionTask).Times(cacheSize)
 	// after *cacheSize* "legit" tasks are fed to our worker, start feeding our worker empty responses.
 	// these will get tossed away immediately after polled, but we still need them so gomock doesn't compain about unexpected calls.
 	// this is because our worker's poller doesn't stop, it keeps polling on the service client as long
 	// as Stop() is not called on the worker
-	s.service.EXPECT().PollForDecisionTask(gomock.Any(), gomock.Any(), callOptions...).Return(&m.PollForDecisionTaskResponse{}, nil).AnyTimes()
+	s.service.EXPECT().PollForDecisionTask(gomock.Any(), gomock.Any(), callOptions()...).Return(&m.PollForDecisionTaskResponse{}, nil).AnyTimes()
 	// this gets called after polled decision tasks are processed, any number of times doesn't matter
-	s.service.EXPECT().RespondDecisionTaskCompleted(gomock.Any(), gomock.Any(), callOptions...).Return(&m.RespondDecisionTaskCompletedResponse{}, nil).AnyTimes()
+	s.service.EXPECT().RespondDecisionTaskCompleted(gomock.Any(), gomock.Any(), callOptions()...).Return(&m.RespondDecisionTaskCompletedResponse{}, nil).AnyTimes()
 	// this is the critical point of the test.
 	// ResetSticky should be called exactly once because our workflow cache evicts when full
 	// so if our worker puts *cacheSize* entries in the cache, it should evict exactly one
-	s.service.EXPECT().ResetStickyTaskList(gomock.Any(), gomock.Any(), callOptions...).DoAndReturn(mockResetStickyTaskList).Times(1)
+	s.service.EXPECT().ResetStickyTaskList(gomock.Any(), gomock.Any(), callOptions()...).DoAndReturn(mockResetStickyTaskList).Times(1)
 
-	workflowWorker := internal.NewWorker(s.service, "test-domain", "tasklist", worker.Options{DisableActivityWorker: true})
+	workflowWorker := internal.NewWorker(s.service, "test-domain", "tasklist", worker.Options{
+		DisableActivityWorker: true,
+		Logger:                zaptest.NewLogger(s.T()),
+	})
 	// this is an arbitrary workflow we use for this test
 	// NOTE: a simple helloworld that doesn't execute an activity
 	// won't work because the workflow will simply just complete

go.mod

@@ -3,7 +3,7 @@ module go.uber.org/cadence
 go 1.13
 
 require (
-	github.com/apache/thrift v0.13.0
+	github.com/apache/thrift v0.0.0-20161221203622-b2a4d4ae21c7
 	github.com/facebookgo/clock v0.0.0-20150410010913-600d898af40a
 	github.com/gogo/protobuf v1.3.2
 	github.com/golang/mock v1.4.4
@@ -21,13 +21,10 @@ require (
 	go.uber.org/goleak v1.0.0
 	go.uber.org/multierr v1.6.0
 	go.uber.org/thriftrw v1.25.0
-	go.uber.org/yarpc v1.53.2
+	go.uber.org/yarpc v1.55.0
 	go.uber.org/zap v1.13.0
 	golang.org/x/lint v0.0.0-20200130185559-910be7a94367
 	golang.org/x/net v0.0.0-20201021035429-f5854403a974
 	golang.org/x/time v0.0.0-20170927054726-6dc17368e09b
 	honnef.co/go/tools v0.0.1-2019.2.3
 )
-
-// yarpc brings up new thrift version, which is not compatible with current codebase
-replace github.com/apache/thrift => github.com/apache/thrift v0.0.0-20161221203622-b2a4d4ae21c7

go.sum

@@ -194,8 +194,8 @@ go.uber.org/thriftrw v1.25.0 h1:x0Omju0vwFn4JniYUqB0w1nycxjE42wNptB7DAtZG/Y=
 go.uber.org/thriftrw v1.25.0/go.mod h1:IcIfSeZgc59AlYb0xr0DlDKIdD7SgjnFpG9BXCPyy9g=
 go.uber.org/tools v0.0.0-20190618225709-2cfd321de3ee h1:0mgffUl7nfd+FpvXMVz4IDEaUSmT1ysygQC7qYo7sG4=
 go.uber.org/tools v0.0.0-20190618225709-2cfd321de3ee/go.mod h1:vJERXedbb3MVM5f9Ejo0C68/HhF8uaILCdgjnY+goOA=
-go.uber.org/yarpc v1.53.2 h1:nj8ZKEmlebPMA9/IoCMUC9qvrB6zQ24sJBhBwORAMD8=
-go.uber.org/yarpc v1.53.2/go.mod h1:MN4UGtPgheIYyT6HAXKMjGI9H736L+7ZuHIyeRpurW8=
+go.uber.org/yarpc v1.55.0 h1:kd9jbG12t6GkSMRzPx8VcgdQxh8hhjSZX85FtSrzgZ0=
+go.uber.org/yarpc v1.55.0/go.mod h1:V2JUPDWHYGNpvyuroYjf0KFjwvBCtcFJLuvZqv7TWA0=
 go.uber.org/zap v1.9.1/go.mod h1:vwi/ZaCAaUcBkycHslxD9B2zi4UTXhF60s6SWpuDF0Q=
 go.uber.org/zap v1.13.0 h1:nR6NoDBgAf67s68NhaXbsojM+2gxp3S1hWkHDl27pVU=
 go.uber.org/zap v1.13.0/go.mod h1:zwrFLgMcdUuIBviXEYEH1YKNaOBnKXsx2IPda5bBwHM=