-
Notifications
You must be signed in to change notification settings - Fork 17
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.
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
[CAPPL-536] Workflow update not picked up by Syncer #1029
Changes from 2 commits
3ba24f4
46388e4
17473fd
1987a00
10352b5
559bf4d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -215,9 +215,31 @@ func (t *triggerExecutableServer) RegisterTrigger(request *capabilitiespb.Trigge | |
} | ||
responseCh, err := t.impl.RegisterTrigger(server.Context(), req) | ||
if err != nil { | ||
// the first message sent to the client will be an ack or error message, this is done in order to syncronize the client and server and avoid | ||
// errors to unregister not found triggers. If the error is not nil, we send an error message to the client and return the error | ||
msg := &capabilitiespb.TriggerResponseMessage{ | ||
Message: &capabilitiespb.TriggerResponseMessage_Response{ | ||
Response: &capabilitiespb.TriggerResponse{ | ||
Error: err.Error(), | ||
}, | ||
}, | ||
} | ||
if err = server.Send(msg); err != nil { | ||
return fmt.Errorf("failed sending error response for trigger registration %s: %w", request, err) | ||
} | ||
return fmt.Errorf("error registering trigger: %w", err) | ||
} | ||
|
||
// Send ACK response to client | ||
msg := &capabilitiespb.TriggerResponseMessage{ | ||
Message: &capabilitiespb.TriggerResponseMessage_Response{ | ||
Response: &capabilitiespb.TriggerResponse{}, | ||
}, | ||
} | ||
if err = server.Send(msg); err != nil { | ||
return fmt.Errorf("failed sending ACK response for trigger registration %s: %w", request, err) | ||
} | ||
|
||
defer func() { | ||
// Always attempt to unregister the trigger to ensure any related resources are cleaned up | ||
err = t.impl.UnregisterTrigger(server.Context(), req) | ||
|
@@ -270,6 +292,17 @@ func (t *triggerExecutableClient) RegisterTrigger(ctx context.Context, req capab | |
return nil, fmt.Errorf("error registering trigger: %w", err) | ||
} | ||
|
||
// In order to ensure the registration is successful, we need to wait for the first message from the server. | ||
// This will be an ack or error message. If the error is not nil, we return an error. | ||
ackMsg, err := responseStream.Recv() | ||
justinkaseman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if err != nil { | ||
return nil, fmt.Errorf("failed to receive registering trigger ack message: %w", err) | ||
} | ||
|
||
if ackMsg.GetResponse().GetError() != "" { | ||
return nil, errors.New(fmt.Sprintf("failed registering trigger: %s", ackMsg.GetResponse().GetError())) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @agparadiso This code is a bit ambiguous I think: what happens if the server sends a response without an ack? We should clearly error in that case as that would mean the protocol has been violated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed to send: |
||
|
||
return forwardTriggerResponsesToChannel(ctx, t.Logger, req, responseStream.Recv) | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to return an error in this case, you already returned it back to the caller successfully, just return nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better yet, I think
return server.Send(msg)
is enoughThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, done 👍🏼