Skip to content

Commit

Permalink
Fix incorrect nil handling in workflowTaskPoller (#1412)
Browse files Browse the repository at this point in the history
What changed?

explicit nil handling in newWorkflowTaskPoller for ldaTunnel
Why?

Golang doesn't handle passing in nil pointer of concrete type directly into the interface. It still considers it not nil, which will cause panic in this case.

How did you test it?

Unit test fail before and pass after the change
  • Loading branch information
shijiesheng authored Dec 12, 2024
1 parent 9ffbb1f commit 2bceb13
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 1 deletion.
10 changes: 9 additions & 1 deletion internal/internal_task_pollers.go
Original file line number Diff line number Diff line change
Expand Up @@ -290,14 +290,22 @@ func newWorkflowTaskPoller(
domain string,
params workerExecutionParameters,
) *workflowTaskPoller {
/*
Explicit nil handling is needed here. Otherwise, poller.ldaTunnel would not return a nil result
*/
var ldaTunnelInterface localDispatcher
if ldaTunnel != nil {
ldaTunnelInterface = ldaTunnel
}

return &workflowTaskPoller{
basePoller: basePoller{shutdownC: params.WorkerStopChannel},
service: service,
domain: domain,
taskListName: params.TaskList,
identity: params.Identity,
taskHandler: taskHandler,
ldaTunnel: ldaTunnel,
ldaTunnel: ldaTunnelInterface,
metricsScope: metrics.NewTaggedScope(params.MetricsScope),
logger: params.Logger,
stickyUUID: uuid.New(),
Expand Down
15 changes: 15 additions & 0 deletions internal/internal_task_pollers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,21 @@ const (
_testIdentity = "test-worker"
)

func Test_newWorkflowTaskPoller(t *testing.T) {
t.Run("success with nil ldaTunnel", func(t *testing.T) {
poller := newWorkflowTaskPoller(
nil,
nil,
nil,
_testDomainName,
workerExecutionParameters{})
assert.NotNil(t, poller)
if poller.ldaTunnel != nil {
t.Error("unexpected not nil ldaTunnel")
}
})
}

func TestLocalActivityPanic(t *testing.T) {
// regression: panics in local activities should not terminate the process
s := WorkflowTestSuite{logger: testlogger.NewZap(t)}
Expand Down

0 comments on commit 2bceb13

Please sign in to comment.