fix: mediator updates #432
Conversation
TimoGlastra
commented
Aug 20, 2021
TimoGlastra
commented
- extract logic for implicit pickup from websocket transport to mediation module
- This will make it easier to move to implicit pickup of non-websocket transports
- add test for restarting recipient agent
- add timeout to return when is connected
- fix issue where services were not reassigned after sorting or filtering
- add persistence for mediator routing keys
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #432 +/- ##
==========================================
+ Coverage 85.66% 86.07% +0.41%
==========================================
Files 249 252 +3
Lines 5351 5394 +43
Branches 796 797 +1
==========================================
+ Hits 4584 4643 +59
+ Misses 767 751 -16
Continue to review full report at Codecov.
|
| const queueService = allServices.find((s) => isDidCommTransportQueue(s.serviceEndpoint)) | ||
|
|
||
| //If restrictive will remove services not listed in schemes list | ||
| if (transportPriority?.restrictive) { | ||
| services.filter((service) => { | ||
| services = services.filter((service) => { |
There was a problem hiding this comment.
@JamesKEbert @burdettadam The services were not being reassigned meaning the sort and filter did not do anything. This should fix it.
This means that currently it will take the http endpoint, send the ping and receive the ping response after which the http endpoint is closed 😄
So if we could merge this rather sooner than later that would be great!
There was a problem hiding this comment.
How embarrassing! Thanks for catching that.
There was a problem hiding this comment.
Whoooops--my bad!
As a side note, the array.sort() method does actually do its operation in place and returns too, so it doesn't necessarily have to be reassigned too, but doesn't hurt to reassign it. :)
There was a problem hiding this comment.
Ah you're right. So weird that filter does not do its operation in place, but sort does. Thanks
Signed-off-by: Timo Glastra <[email protected]>
There was a problem hiding this comment.
These are great, thanks @TimoGlastra! I am okay merging, but definitely would like to remove our need to externalize the connectionRecords to the transports.
| this.transportTable.forEach((socket) => { | ||
| socket.removeEventListener('message', this.handleMessageEvent) | ||
| socket.close() | ||
| }) | ||
| } | ||
|
|
||
| public async sendMessage(outboundPackage: OutboundPackage) { | ||
| const { payload, endpoint } = outboundPackage | ||
| const { payload, endpoint, connection } = outboundPackage |
There was a problem hiding this comment.
I personally would really like to see this be connectionId as a string vs sending the whole connectionRecord, since that starts to really tie our transports into the inner workings of the Framework more than they are now.
There was a problem hiding this comment.
That's an interesting point. But connectionRecrod is already part of the public API of the framework 🤔 If you think we're exposing too much framework internal info via connectionRecord, maybe, we should introduce some domain object hiding unnecessary details.
| }) | ||
|
|
||
| await this.openMediationWebSocket(mediator) | ||
| } |
There was a problem hiding this comment.
Shouldn't those methods be part of the transport layer rather than inside the core of the framework? Or, it's just temporary?
There was a problem hiding this comment.
What it does is sending a trust ping message with a restriction to only use websocket. The websocket will be kept alive autmoatically (I'd like to make the keep alive more explicit)
I'm not sure yet how we could nicely integrate this into the transport layer. Would be nice to not make it WebSocket specific (so just send a ping and keep the socket alive), but we need to specify somewhere which transports are capable of this, as e.g. HTTP isn't.
Maybe a transport should be able to specify whether it can send multiple messages (WebSocket is ∞, while HTTP is 1 request, 1 response). That way we wouldn't have to specify WS explicitly, but rather whether we want a transport that can stay open (for implicit pickup)
I'm going to think about it for a while. We can have a discussion about it in the WG call
There was a problem hiding this comment.
I have opened #435. Please leave your thoughts :)
| this.transportTable.forEach((socket) => { | ||
| socket.removeEventListener('message', this.handleMessageEvent) | ||
| socket.close() | ||
| }) | ||
| } | ||
|
|
||
| public async sendMessage(outboundPackage: OutboundPackage) { | ||
| const { payload, endpoint } = outboundPackage | ||
| const { payload, endpoint, connection } = outboundPackage |
There was a problem hiding this comment.
That's an interesting point. But connectionRecrod is already part of the public API of the framework 🤔 If you think we're exposing too much framework internal info via connectionRecord, maybe, we should introduce some domain object hiding unnecessary details.
|
I see there is some bugfix as part of the PR so we should merge it. |
Signed-off-by: Timo Glastra <[email protected]>
