Matching support for cancelling outstanding pollers #353
Conversation
service/frontend/handler.go
Outdated
| PollRequest: pollRequest, | ||
| }) | ||
| if err != nil { | ||
| // First check if this err is due to context cancellation. This means client connection to frontend is closed. | ||
| if ctx.Err() == context.Canceled { | ||
| // Our rpc stack does not propagates context cancellation to the other service. Lets make an explicit |
There was a problem hiding this comment.
can you move this cancel logic into its own method ?
service/matching/taskListManager.go
Outdated
|
|
||
| pollerID, ok := ctx.Value(pollerIDKey).(string) | ||
| childCtx, cancel := context.WithCancel(ctx) | ||
| if ok { |
There was a problem hiding this comment.
since the else{} part is only for backward compatibility, this code can be refactored to remove the else entirely i.e. move the context.WithCancel(ctx) within the if ok branch and initialize childCtx = ctx outside the if
service/frontend/handler.go
Outdated
| if ctx.Err() == context.Canceled { | ||
| // Our rpc stack does not propagates context cancellation to the other service. Lets make an explicit | ||
| // call to matching to notify this poller is gone to prevent any tasks being dispatched to zombie pollers. | ||
| err = wh.matching.CancelOutstandingPoll(nil, &m.CancelOutstandingPollRequest{ |
There was a problem hiding this comment.
is it standard to use nil as context if there is no context? or is it better to use context.Background()?
There was a problem hiding this comment.
updated it to context.Background.
service/matching/taskListManager.go
Outdated
| c.outstandingPollsLock.Unlock() | ||
| cancel() | ||
| }() | ||
| } else { |
There was a problem hiding this comment.
This is just for backwards compatibility.
| tCtx, err := e.getTask(ctx, taskList) | ||
| // Add frontend generated pollerID to context so tasklistMgr can support cancellation of | ||
| // long-poll when frontend calls CancelOutstandingPoll API | ||
| pollerCtx := context.WithValue(ctx, pollerIDKey, pollerID) |
There was a problem hiding this comment.
would it possible that pollerID is empty (not supplied by frontend from older version or whatever reason)? Do we need to skip adding the empty pollerID to ctx if it is the case?
There was a problem hiding this comment.
Good point. I have added protection in tasklistmgr.gettask to protect against empty pollerID.
RPC stack used by Cadence does not support cancel propagation through context and this results in tasks being dispatched to zombie pollers when client connection is closed. Updated mathcing engine api for PollForDecisionTask and PollForActivityTask to alsoo take in pollerID. Also added new API on matching engine to CancelOutstandingPolls given pollerID. Frontend handler for Poll API generates pollerID before calling the corresponding API on matching engine, if the call fails due to context cancellation it calls the CancelOutstandingPoll API on the matching engine to unblock corresponding poll requests.
samarabbas
force-pushed
the
zombie-poller
branch
from
f8c9ab0
to
211a3a7
Compare
RPC stack used by Cadence does not support cancel propagation through
context and this results in tasks being dispatched to zombie pollers
when client connection is closed.
Updated mathcing engine api for PollForDecisionTask and
PollForActivityTask to alsoo take in pollerID. Also added new API on
matching engine to CancelOutstandingPolls given pollerID.
Frontend handler for Poll API generates pollerID before calling the
corresponding API on matching engine, if the call fails due to context
cancellation it calls the CancelOutstandingPoll.
Fixes Issue #254