DAOS-7198 control: Use join request context instead of timeout #5380
Conversation
In the MS joinLoop, a short timeout was used to avoid blocking the loop if a join request handler exited before receiving the batched join response. Rather than relying on an arbitrary timeout value, we should instead pass in the join request's context so that we can correctly wait for the handler to receive the request or for the context to be canceled. Either way, it won't block the join loop indefinitely. Signed-off-by: Michael MacDonald <mjmac.macdonald@intel.com>
|
Test stage Functional Hardware Small completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-5380/4/display/redirect |
| @@ -388,6 +387,7 @@ func (svc *mgmtSvc) Join(ctx context.Context, req *mgmtpb.JoinReq) (*mgmtpb.Join | |||
| bjr := &batchJoinRequest{ | |||
| JoinReq: *req, | |||
| peerAddr: replyAddr, | |||
| joinCtx: ctx, | |||
There was a problem hiding this comment.
So here we are ignoring the language's recommendation not to include contexts inside structs: https://blog.golang.org/context-and-structs . It should be noted that there is discussion to relax that recommendation golang/go#22602 and given this is a temporary storage in a request it seems okay.
There was a problem hiding this comment.
Yes, the guidance is not really about having a context in a struct -- i.e. it's not that there is something inherently unsafe about this, it's that storing a context in a long-lived struct rather than accepting a context in that struct's methods is an anti-pattern for various reasons explained elsewhere.
From the second link:
While we've told people not to add contexts to structs, I think that guidance is over-aggressive. The real advice is not to store contexts. They should be passed along like parameters. But if the struct is essentially just a parameter, it's okay. I think this concern can be addressed with package-level documentation and examples.
The batchJoinRequest struct is indeed a parameter and it's used to convey information from the Join RPC handler into the joinLoop.
In the MS joinLoop, a short timeout was used to avoid blocking the loop if a join request handler exited before receiving the batched join response. Rather than relying on an arbitrary timeout value, we should instead pass in the join request's context so that we can correctly wait for the handler to receive the request or for the context to be canceled. Either way, it won't block the join loop indefinitely. Master-PR: #5380 Signed-off-by: Michael MacDonald <mjmac.macdonald@intel.com>
In the MS joinLoop, a short timeout was used to avoid blocking the loop if a join request handler exited before receiving the batched join response. Rather than relying on an arbitrary timeout value, we should instead pass in the join request's context so that we can correctly wait for the handler to receive the request or for the context to be canceled. Either way, it won't block the join loop indefinitely. Master-PR: #5380 Signed-off-by: Michael MacDonald <mjmac.macdonald@intel.com>
In the MS joinLoop, a short timeout was used to avoid blocking
the loop if a join request handler exited before receiving the
batched join response. Rather than relying on an arbitrary
timeout value, we should instead pass in the join request's
context so that we can correctly wait for the handler to receive
the response or for the context to be canceled. Either way, it
won't block the join loop indefinitely.