refactor: make a connection with mediator asynchronously #231

jakubkoci · 2021-04-10T15:12:13Z

The previous implementation assumed that a connection with a mediator is made “synchronously “ via HTTP transport. By “synchronously” I mean that the provision method handled every step of making a connection and waiting for the response message assuming HTTP request-response communication. Now it should be independent on the transport layer. It just accepts the invitation and waits until the connection is made regardless of how the messages are exchanged.

I needed to do the following changes:

Add return_route parameter to outbound when there is no inbound connection (there is no service with endpoint other than didcomm:transport/queue)
Add keylist-update-response message and handler, just with naive implementation.
Extract outboundTransporter as direct dependency in Agent’s constructor.

Next steps on the way to support WebSockets transport I would like to do:

Extract also inboundTransporter from Agent’s constructor. (I can perhaps do it as part of this PR, but the changes started growing and I wanted to share what I have now.)
Take outbound endpoint from DidDoc.
Add WebSocket transporters.
Add TransportService to keep info about current transport channels.
Allow to set more inbound and outbound transporters.

Next steps I see we will need to do someday in near future but not necessarily before or right after adding the WebSockets:

Update download messages functionality to process batch message and to remove sendAndReceiveMessage that is still implemented in the old way relaying on synchronicity of HTTP request-response.

BREAKING CHANGE: extracts outbound transporter from Agent's constructor.

Signed-off-by: Jakub Koci <[email protected]>

codecov-io · 2021-04-10T15:17:34Z

Codecov Report

Merging #231 (c5d5908) into main (2ae11f7) will decrease coverage by 0.11%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##             main     #231      +/-   ##
==========================================
- Coverage   89.06%   88.95%   -0.12%     
==========================================
  Files         174      176       +2     
  Lines        3302     3358      +56     
  Branches      364      370       +6     
==========================================
+ Hits         2941     2987      +46     
- Misses        358      368      +10     
  Partials        3        3

Impacted Files	Coverage Δ
...modules/routing/services/ProviderRoutingService.ts	`10.41% <12.50%> (-1.22%)`	⬇️
...c/modules/routing/handlers/KeylistUpdateHandler.ts	`54.54% <33.33%> (-1.02%)`	⬇️
src/agent/MessageSender.ts	`92.30% <77.77%> (+2.30%)`	⬆️
...s/routing/messages/KeylistUpdateResponseMessage.ts	`85.71% <85.71%> (ø)`
src/agent/Agent.ts	`100.00% <100.00%> (ø)`
src/agent/Dispatcher.ts	`96.42% <100.00%> (+4.12%)`	⬆️
src/modules/connections/ConnectionsModule.ts	`64.61% <100.00%> (-7.76%)`	⬇️
...modules/connections/repository/ConnectionRecord.ts	`97.18% <100.00%> (+0.08%)`	⬆️
.../modules/connections/services/ConnectionService.ts	`100.00% <100.00%> (ø)`
src/modules/routing/RoutingModule.ts	`92.59% <100.00%> (+0.28%)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ae11f7...c5d5908. Read the comment docs.

TimoGlastra

I like these changes @jakubkoci! :)

The only thing I'm not sure about is the addition of sendAndReceiveMessage to the OutboundTransporter interface. Maybe you can clarify a bit on that

samples/__tests__/e2e.test.ts

TimoGlastra · 2021-04-10T19:36:10Z

src/agent/MessageSender.ts

    outboundMessage.payload.setReturnRouting(ReturnRouteTypes.all)

    const outboundPackage = await this.envelopeService.packMessage(outboundMessage)
-    const inboundPackedMessage = await this.outboundTransporter.sendMessage(outboundPackage, true)
+    const inboundPackedMessage = await this.outboundTransporter.sendAndReceiveMessage(outboundPackage)


I'm not fully understanding. Why did you add a sendAndReceiveMessage to the outbound transporter?

Good question. I wanted to preserve the current behavior of await agent.routing.downloadMessages().

First, I thought that using sendMessage(outboundPackage, true) would be good enough. The problem is that in HTTP outbound we now pass the response immediately to the agent calling this.agent.receiveMessage(wireMessage). It means that the batch response is not processed inside downloadMessages anymore.

This is related to the problem we discussed in our last AFJ call about dispatching messages to the agent from a message handler. I listed as last point in the description:
"Update download messages functionality to process batch message and to remove sendAndReceiveMessage that is still implemented in the old way relaying on synchronicity of HTTP request-response."

I'm thinking if we could implement the solution as part of this PR. I usually tend to work in smaller batches. Maybe prioritize it at least and do it before WebSockets 🤷‍♂️ Maybe it's not so big change 🤔

TimoGlastra · 2021-04-10T19:38:07Z

src/modules/routing/RoutingModule.ts

+      const { message: connectionRequest } = await this.connectionService.createRequest(connectionRecord.id)
+
+      const outboundMessage = createOutboundMessage(connectionRecord, connectionRequest, connectionRecord.invitation)
+      outboundMessage.payload.setReturnRouting(ReturnRouteTypes.all)


You're already setting return routing if the connection has no inbound endpoint. this shouldn't be needed right?

I had to move it into Dispatcher, where is not called because of the manual invitation acceptance process here. When return_route is set, we don't call sendMessage where I had it previously in #218

TimoGlastra · 2021-04-10T19:41:04Z

src/modules/routing/handlers/KeylistUpdateResponseHandler.ts

+  public supportedMessages = [KeylistUpdateResponseMessage]
+
+  public async handle(messageContext: HandlerInboundMessage<KeylistUpdateResponseHandler>) {
+    // TODO It should handle the response when agent calls `await this.consumerRoutingService.createRoute(connectionRecord.verkey)` and notify about the result.


Why aren't we doing anything with the response? Are you going to address this in a separate PR?

We think should do it in separate PR which would also add the step to wait for the response after calling createRoute. I didn't plan to add this at first, but I had to when I set return_route and update HTTP inbound transporter. I wasn't 100 % sure what to do here and it seemed kind of out of the scope of this PR.

I created #234 to address this.

src/modules/routing/messages/KeylistUpdateResponseMessage.ts

src/modules/routing/messages/RoutingMessageType.ts

TimoGlastra · 2021-04-10T19:44:58Z

src/modules/routing/services/ProviderRoutingService.ts

+    const { connection, message } = messageContext
+
+    if (!connection) {
+      // TODO We could eventually remove this check if we do it at some higher level where we create messageContext that must have a connection.


Yup would be nice to be able to declare if a connection is required for a handler or something...

Signed-off-by: Jakub Koci <[email protected]>

jakubkoci · 2021-04-11T10:30:11Z

Thanks, @TimoGlastra for the good suggestions. I see we have 3 different areas to address (ordered by priority from my perspective):

sendAndReceiveMessage
calling outboundMessage.payload.setReturnRouting(ReturnRouteTypes.all) at two places
missing implementation of KeylistUpdateResponseHandler

I don't think we have to solve them all here. I'm quite curious about the solution to the 1. issue so I'm going to look at it briefly at least, considering prioritizing that.

Signed-off-by: Jakub Koci <[email protected]>

jakubkoci · 2021-04-13T08:38:44Z

I added an event emitter to Agent and Batch message handler to emit incoming messages to Agent. That allowed to remove sendAndReceiveMessage.

I'm not saying it's perfect, but I'm quite happy with this for now. What do you think @TimoGlastra?

jakubkoci · 2021-04-13T08:48:16Z

I updated the PR name according to conventional commits. I guess this also brings some breaking change in agent API which I expressed in the description of the PR. That can go into the commit message when merging.

TimoGlastra

Nice! I like it.

With the addition of the new event emitter I think it'll be good to look at a global event emitter rather sooner than later

TimoGlastra · 2021-04-13T09:28:48Z

src/modules/routing/handlers/BatchHandler.ts

+    forwardedMessages.forEach((message) => {
+      this.eventEmitter.emit('agentMessage', message.message)
+    })


Do you think it will cause problems that all messages are emitted at the same time? E.g. If we receive 10 messages, they all get processed at the same time, meaning message 10 could be processed before message 5. It could also mean we fetch a record, another received message updated the record in the meantime and then we overwrite the new data.

I'm not sure if we want to take such things into account. A message queue could solve this, but being able to handle message in parallel would be nice. ACA-Py handles this by fetching a record 'for-update' which means it'll lock other sessions from writing until you are done. Reading is always possible.

Yeah. I'm afraid that the problem can occur. My first idea was also a message queue. You're right that it could be convenient to handle messages in parallel in some cases. I'm surprised that ACA-Py decided to go with locking of objects which sounds quite sophisticated to me, but maybe it's the only option 🤷‍♂️

jakubkoci · 2021-04-13T20:50:44Z

With the addition of the new event emitter I think it'll be good to look at a global event emitter rather sooner than later

Agree. It could solve even more problems like:

dispatching incoming messages (as we're already using it for here)
independent communication between modules
notification about createRoute result Wait for a response when creating a route for a new connection in a mediator. #234

…foundation#231) * add `return_route` parameter to outbound when there is no inbound connection (no service with endpoint other than `didcomm:transport/queue`) * add keylist-update-response message and handler with a naive implementation * add batch message handler with agent event emitter BREAKING CHANGE: extracts outbound transporter from Agent's constructor. Signed-off-by: Jakub Koci <[email protected]>

jakubkoci requested a review from a team as a code owner April 10, 2021 15:12

Make a connection with mediator asynchronously

c5d5908

Signed-off-by: Jakub Koci <[email protected]>

TimoGlastra requested changes Apr 10, 2021

View reviewed changes

jakubkoci added 2 commits April 11, 2021 11:46

Update keylist update messages

555ddff

Signed-off-by: Jakub Koci <[email protected]>

Add check for response to http outbound

6fa28ca

Signed-off-by: Jakub Koci <[email protected]>

Add batch message handler with agent event emitter

65f8728

Signed-off-by: Jakub Koci <[email protected]>

jakubkoci changed the title ~~Make a connection with mediator asynchronously~~ refactor: make a connection with mediator asynchronously Apr 13, 2021

TimoGlastra approved these changes Apr 13, 2021

View reviewed changes

jakubkoci merged commit bafa839 into openwallet-foundation:main Apr 13, 2021

jakubkoci mentioned this pull request Apr 23, 2021

Add support for WebSockets #250

Closed

TimoGlastra mentioned this pull request May 6, 2021

Race condition with connection / credential offer #259

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: make a connection with mediator asynchronously #231

refactor: make a connection with mediator asynchronously #231

jakubkoci commented Apr 10, 2021 •

edited

Loading

codecov-io commented Apr 10, 2021 •

edited

Loading

TimoGlastra left a comment

TimoGlastra Apr 10, 2021

jakubkoci Apr 11, 2021 •

edited

Loading

TimoGlastra Apr 10, 2021

jakubkoci Apr 11, 2021 •

edited

Loading

TimoGlastra Apr 10, 2021

jakubkoci Apr 11, 2021

TimoGlastra Apr 10, 2021

jakubkoci commented Apr 11, 2021

jakubkoci commented Apr 13, 2021

jakubkoci commented Apr 13, 2021 •

edited

Loading

TimoGlastra left a comment

TimoGlastra Apr 13, 2021

jakubkoci Apr 13, 2021

jakubkoci commented Apr 13, 2021

refactor: make a connection with mediator asynchronously #231

refactor: make a connection with mediator asynchronously #231

Conversation

jakubkoci commented Apr 10, 2021 • edited Loading

codecov-io commented Apr 10, 2021 • edited Loading

Codecov Report

TimoGlastra left a comment

Choose a reason for hiding this comment

TimoGlastra Apr 10, 2021

Choose a reason for hiding this comment

jakubkoci Apr 11, 2021 • edited Loading

Choose a reason for hiding this comment

TimoGlastra Apr 10, 2021

Choose a reason for hiding this comment

jakubkoci Apr 11, 2021 • edited Loading

Choose a reason for hiding this comment

TimoGlastra Apr 10, 2021

Choose a reason for hiding this comment

jakubkoci Apr 11, 2021

Choose a reason for hiding this comment

TimoGlastra Apr 10, 2021

Choose a reason for hiding this comment

jakubkoci commented Apr 11, 2021

jakubkoci commented Apr 13, 2021

jakubkoci commented Apr 13, 2021 • edited Loading

TimoGlastra left a comment

Choose a reason for hiding this comment

TimoGlastra Apr 13, 2021

Choose a reason for hiding this comment

jakubkoci Apr 13, 2021

Choose a reason for hiding this comment

jakubkoci commented Apr 13, 2021

jakubkoci commented Apr 10, 2021 •

edited

Loading

codecov-io commented Apr 10, 2021 •

edited

Loading

jakubkoci Apr 11, 2021 •

edited

Loading

jakubkoci Apr 11, 2021 •

edited

Loading

jakubkoci commented Apr 13, 2021 •

edited

Loading