Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DAOS-7198 control: Use join request context instead of timeout #5380

Merged
merged 1 commit into from
Apr 9, 2021
Merged

DAOS-7198 control: Use join request context instead of timeout #5380

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/control/server/mgmt_system.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,13 +139,13 @@ func getPeerListenAddr(ctx context.Context, listenAddrStr string) (*net.TCPAddr,
const (
groupUpdateInterval = 500 * time.Millisecond
batchJoinInterval = 250 * time.Millisecond
joinRespTimeout = 10 * time.Millisecond
)

type (
batchJoinRequest struct {
mgmtpb.JoinReq
peerAddr *net.TCPAddr
joinCtx context.Context
respCh chan *batchJoinResponse
}

Expand Down Expand Up @@ -217,12 +217,11 @@ func (svc *mgmtSvc) joinLoop(parent context.Context) {

svc.log.Debugf("sending %d join responses", len(joinReqs))
for i, req := range joinReqs {
ctx, cancel := context.WithTimeout(parent, joinRespTimeout)
defer cancel()

select {
case <-ctx.Done():
svc.log.Errorf("failed to send join response: %s", ctx.Err())
case <-parent.Done():
svc.log.Errorf("joinLoop shut down before response sent: %s", parent.Err())
case <-req.joinCtx.Done():
svc.log.Errorf("failed to send join response: %s", req.joinCtx.Err())
case req.respCh <- joinResps[i]:
}
}
Expand Down Expand Up @@ -388,6 +387,7 @@ func (svc *mgmtSvc) Join(ctx context.Context, req *mgmtpb.JoinReq) (*mgmtpb.Join
bjr := &batchJoinRequest{
JoinReq: *req,
peerAddr: replyAddr,
joinCtx: ctx,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

respCh: make(chan *batchJoinResponse),
}

Expand Down