fix: Handling grpc cancellation edge-case:: Cancelling at step START

[oandreeva-nv]

What does the PR do?

This PR fixes an issue with a gRPC ModelInferHandler stopping accepting requests after a number of cancellation received.

Main root-cause and pre-conditions before hand:
At some point cancellation notification is received at step START. When this happens, we never skip this block

Since, we never went skip it, we don't create new state for future incoming requests, i.e. call StartNewRequest

Thus, completion queue becomes exhausted at some point. gRPC requests come, but there is nothing that accepts it on Triton's side.

Introduced changes make sure we create new request handler in those situations.
Added test logic:

I start server and send large amount of inference requests and cancel them right away. Pre fix, the clear identification that there are no ModelInferHandler's for any in-coming request is the server stops logging "New request handler for ModelInfer", i.e
grep -c "New request handler for ModelInfer" doesn't change. In all pre-fix scenarious it happens after
"Cancellation notification received for ModelInferHandler, rpc_ok=1, context 0, [0-9]* step START" was logged 4 times, 2 times for 1 request. Since we start 2 ModelInferHandler's threads initially, that make sense, as no new Infer handlers were created to handle incoming requests and Triton just keeps processing what it already has.

After fix, StartNewRequest is called properly and "New request handler for ModelInfer" keeps growing, as well as "Cancellation notification received for ModelInferHandler, rpc_ok=1, context 0, [0-9]* step START".

Checklist`

• PR title reflects the change and is of format <commit_type>: <Title>
• Changes are described in the pull request.
• Related issues are referenced.
• Populated github labels field
• Added test plan and verified test passes.
• Verified that the PR passes existing CI.
• Verified copyright is correct on all changed files.
• Added succinct git squash message before merging ref.
• All template sections are filled out.
• Optional: Additional screenshots for behavior/output changes with before/after.

Commit Type:

Check the conventional commit type
box here and add the label to the github PR.

• build
• ci
• docs
• feat
• fix
• perf
• refactor
• revert
• style
• test

Related PRs:

Where should the reviewer start?

Test plan:

• CI Pipeline ID:
  pre-fix: 15596577
  post-fix: 15596692

Caveats:

Background

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

[nnshah1]

question: what does the second clause here imply?

!= Steps::Cancelled ?

[oandreeva-nv]

To avoid calling StartNewRequest twice, at first we fall into HandleCancellation and go through this block, which returns true for resume, so we will go into if (state->IsGrpcContextCancelled()) loop for the second time but this time state->context_->step_ is CANCELLED

[GuanLuo]

Late to the game, but what is the reasoning of not moving the original "StartNewRequest() if at START" to before handling the cancellation? Although I think other code needs to be moved around as well.

[oandreeva-nv]

I am not 100% aware of all underlying processes, meaning state->step_ and state->context_->step_ combinations. This change helps to address the bug with known symptoms. Refactoring if the Process logic needs proper time and testing IMHO

[GuanLuo]

@kthui thoughts? If feasible, this can be done as follow-up and by someone else. Want to make sure if there is room for improvement.

[kthui]

Yes, I think there is definitely room for improvement/refactoring, i.e. I think the if (shutdown) { ... } could also be moved into the if (state->step_ == Steps::START) { ... } else ... block, so all procedures for Steps::START would be inside the if (state->step_ == Steps::START) { ... } block, but it can be done as a follow-up later.

[oandreeva-nv]

Jira ticket: DLIS-6831

[nnshah1]

LGTM - had question on the condition

[tanmayv25]

I think we would need similar fix for ModelStreamInferHandler.