[CAPPL-20] Support per-method handlers in GatewayConnector #14367
Conversation
bolekk
changed the title
bolekk
force-pushed
the
feature/CAAPL-20-gateway-connector-add-handler
branch
3 times, most recently
from
7cbd5dd to
5046485
Compare
bolekk
requested review from
samsondav and
justinkaseman
and removed request for
a team
Making GatewayConnector compatible with the new design, where each capability is able to add its own handler independently.
bolekk
force-pushed
the
feature/CAAPL-20-gateway-connector-add-handler
branch
from
5046485 to
8138054
Compare
bolekk
requested review from
KuphJr,
DavidOrchard,
cedric-cordenier and
jinhoonbang
| @@ -76,8 +77,8 @@ type gatewayState struct { | |||
| wsClient network.WebSocketClient | |||
| } | |||
|
|
|||
| func NewGatewayConnector(config *ConnectorConfig, signer Signer, handler GatewayConnectorHandler, clock clockwork.Clock, lggr logger.Logger) (GatewayConnector, error) { | |||
| if config == nil || signer == nil || handler == nil || clock == nil || lggr == nil { | |||
| func NewGatewayConnector(config *ConnectorConfig, signer Signer, clock clockwork.Clock, lggr logger.Logger) (GatewayConnector, error) { | |||
There was a problem hiding this comment.
why not give it an array of handlers?
There was a problem hiding this comment.
We don't have them when the connector is constructed.
There was a problem hiding this comment.
If we had a single handler before for calling the connector, why wouldn't we have one still? Is it because the service_wrapper is being created and it won't be ready at connector construction time?
There was a problem hiding this comment.
We had one handler ready here before only because in Functions the Connector object was created by a job spec. Now that you moved it to be a core service, it's not the case any more. Core services (now including Gateway Connector) are all created first, before iterating over job specs and creating services that they need. In the re-design proposal (see doc I shared earlier today), each standard capability will add its own connector handler, when neccesary.
| @@ -125,6 +126,22 @@ func NewGatewayConnector(config *ConnectorConfig, signer Signer, handler Gateway | |||
| return connector, nil | |||
| } | |||
|
|
|||
| func (c *gatewayConnector) AddHandler(methods []string, handler GatewayConnectorHandler) error { | |||
There was a problem hiding this comment.
Maybe addHandlers that takes an array?
There was a problem hiding this comment.
Each capability will only add one handler (to support itself).
| return errors.New("cannot add a nil handler") | ||
| } | ||
| for _, method := range methods { | ||
| if _, exists := c.handlers[method]; exists { |
There was a problem hiding this comment.
why aren't you getting index from the range and then using that on c.handlers?
There was a problem hiding this comment.
c.handlers map is indexed by strings, not integers. Also here I only check for existence before adding later in the subsequent loop.
|
| @@ -194,9 +216,6 @@ func (c *gatewayConnector) reconnectLoop(gatewayState *gatewayState) { | |||
| func (c *gatewayConnector) Start(ctx context.Context) error { | |||
| return c.StartOnce("GatewayConnector", func() error { | |||
| c.lggr.Info("starting gateway connector") | |||
| if err := c.handler.Start(ctx); err != nil { | |||
There was a problem hiding this comment.
I don't see any tests that had to change because the Start and Close were removed. How is it tested that the all the handlers are started and stopped?
There was a problem hiding this comment.
These two lines were removed from connector_test.go:
handler.On("Start", mock.Anything).Return(nil)
handler.On("Close").Return(nil)
|
I think I see that Start happens after all the handlers are added. Is there a race condition where an AddHandler could be called after the Start and it would never get started? Do we need a guard for that? |
Handlers are not being started and closed by the Connector any more. They can be added at any time, even after Start()-ing the Connector. It's up to the owner (i.e. the capability) now to manage its handler's lifecycle. |
Making GatewayConnector compatible with the new design, where each capability is able to add its own handler independently.