add gateway handler and http client for outgoing messages #14536
Conversation
jinhoonbang
commented
Sep 24, 2024
jinhoonbang
commented
- successor to implement HTTP target capability and connector handler #14491 - base branch will be changed to develop when the PR is merged
- adds a gateway handler that handles messages from the capability node. It calls HTTP client to deliver target messages to external endpoints and sends a response back to the capability node.
- The handler function is non-blocking
Co-authored-by: Street <[email protected]>
…ssages. pending rate limiting and unit tests.
jinhoonbang
requested review from
krehermann,
DavidOrchard,
bolekk,
MStreet3,
cedric-cordenier and
justinkaseman
and removed request for
a team and
krehermann
| // this handle method must be non-blocking | ||
| // send response to node (target capability) async | ||
| // if there is a non-HTTP error (e.g. malformed request), send payload with success set to false and error messages | ||
| go func() { |
There was a problem hiding this comment.
Is this how other handlers ensure non-blocking requests?
There's a few things that feel smelly here: 1) the untracked goroutine, and 2) the disconnected context
If anything upstream were to cancel the context, that could feed into cancelling the request too, which doesn't feel right.
There was a problem hiding this comment.
there is one other handler in functions but it doesn't make any external networking calls so it's not implemented as a async task like this. Just for context, the non-blocing pattern using goroutine was suggested here - #14448 (comment)
Although the goroutines are untracked, the goroutine here should always terminate eventually because context with timeout is passed to both SendToNode and sendHTTPMessageToClient. I don't think I understood the concern around disconnected context here. If upstream (gateway service) cancels the context, then the request from target can eventually be cancelled and target capability will timeout resulting in workflow failure, which seems ok.
There was a problem hiding this comment.
I think it's probably not a concern if we aren't crossing any network boundaries or operating in the context of a network handler; in the past when I've implemented something like this in the context of the handler, I've experienced unexpected errors because the server handler will return, and then the server will cancel the context, thus propagating to this goroutine.
How about using context.WithoutCancel(ctx) and then adding a timeout to it? That would ensure the goroutine will respect the timeout, but won't allow the upstream context to cancel the request.
I also think it's worth tracking the goroutine so that if we cancel the handler, we'll also cancel any ongoing requests.
| } | ||
|
|
||
| func (h *handler) Close() error { | ||
| h.wg.Wait() |
There was a problem hiding this comment.
Hmmm... do we need a way to signal to ongoing requests to close here? If their timeout is quite long, then waiting for them to expire could mean we wait for a long time.
Happy for this to be a follow up though
There was a problem hiding this comment.
I am not sure if it's completely necessary since timeout will be in the order of seconds for typical HTTP requests. This reminds me though we should validate the timeout on the capability side to have a reasonable max value.
|
jinhoonbang
deleted the
CM-470-implement-gateway-handler-and-http-client
branch
