Skip to content
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

fix: no return routing and wait for ping #946

Merged
merged 3 commits into from
Jul 14, 2022

Conversation

TimoGlastra
Copy link
Contributor

This PR adds some improvements to return routing. Mainly it adds two things:

  1. Messages returned as a http response are now emitted using an event, meaning the sendMessage await won't wait for the returned message to be processed. This could lead to a chain of send / receive messages that would take a long time to resolve. Especially if the socket was kept open because no response was returned this could lead to unwanted behaviour. (see fix: return if return route but no response acapy#1853 for more context on socket staying open)
  2. Do not include the return route decorator if not response is expected. The return route decorator is automatically added if we don't have an endpoint. This only happens with the mediator in most cases. What would happen is that the trust ping was sent to complete the connection, and we didn't receive a response. This would lead to the socket staying open for 15 seconds, before the mediation request message was sent. Now if the return route is already set (in this case to none) we won't add it in the message sender..

As mentioned in openwallet-foundation/acapy#1853, this reduces the connection + request mediation flow time from ~17 seconds to around 1.5 seconds :)

@TimoGlastra TimoGlastra requested a review from a team as a code owner July 13, 2022 10:47
@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2022

Codecov Report

Merging #946 (cf22cdb) into main (60ee0e5) will decrease coverage by 0.00%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##             main     #946      +/-   ##
==========================================
- Coverage   87.68%   87.67%   -0.01%     
==========================================
  Files         475      475              
  Lines       11406    11414       +8     
  Branches     1937     1938       +1     
==========================================
+ Hits        10001    10007       +6     
- Misses       1338     1340       +2     
  Partials       67       67              
Impacted Files Coverage Δ
.../core/src/modules/connections/ConnectionsModule.ts 68.50% <33.33%> (-0.86%) ⬇️
...ckages/core/src/transport/HttpOutboundTransport.ts 13.63% <50.00%> (+2.00%) ⬆️
packages/core/src/agent/MessageSender.ts 84.61% <100.00%> (ø)
.../connections/handlers/ConnectionResponseHandler.ts 82.35% <100.00%> (+1.10%) ⬆️
...connections/handlers/DidExchangeResponseHandler.ts 81.81% <100.00%> (+0.86%) ⬆️
.../modules/connections/services/ConnectionService.ts 87.14% <100.00%> (ø)

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 60ee0e5...cf22cdb. Read the comment docs.

Copy link
Contributor

@JamesKEbert JamesKEbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great Timo!
You mentioned during the WG call this morning that it does seem a little hacky. Would it be appropriate to add a new decorator or add to the return route decorator to address this specific use case, as that way it might not be as hacky? Thoughts?

@TimoGlastra
Copy link
Contributor Author

Would it be appropriate to add a new decorator or add to the return route decorator to address this specific use case, as that way it might not be as hacky?

Could you elaborate a bit on what you mean?

@TimoGlastra TimoGlastra enabled auto-merge (squash) July 14, 2022 16:11
@JamesKEbert
Copy link
Contributor

JamesKEbert commented Jul 14, 2022

Would it be appropriate to add a new decorator or add to the return route decorator to address this specific use case, as that way it might not be as hacky?

Could you elaborate a bit on what you mean?

Yeah, so let's say that in theory the other agents wants to send us a message after a trust ping/connection completion, they potentially could use the HTTP reply, so it's not that we don't want them sending a response it's that they aren't closing the socket in a timely manner when they don't want to send a response.
So, it might be more appropriate/less-hacky to add to the ~transport decorator with a field like "keep_alive" or "keep_socket_alive" with a boolean value:

"~transport": {
    "return_route": "all",
    "keep_alive: "false"
}

This field to indicate that we don't want the other party to keep the socket open unless they intend to send a response/another message.
May or may not be a good idea. Thoughts?

@TimoGlastra
Copy link
Contributor Author

TimoGlastra commented Jul 14, 2022

Interesting. Well the question arises for me, what does keep-alive mean then. This implies a message is always instantly processed and a message may be returned directly, or the socket will be closed. Maybe we should approach it more as the keep alive header in HTTP: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Keep-Alive?

I think ACA-Py's implementation shouldn't hang on to the http connection after a message has been processed, which means this wouldn't be an issue anymore.

For now I'll merge this as is as it's an improvement in itself. Let's keep looking at other options.

@TimoGlastra TimoGlastra merged commit f48f3c1 into openwallet-foundation:main Jul 14, 2022
@JamesKEbert
Copy link
Contributor

Good thoughts!

Maybe we should approach it more as the keep alive header in HTTP: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Keep-Alive?

Are you saying we should approach it as in the meaning behind the "keep_alive" field in the transport decorator should more mimic the HTTP keep alive header or as in we should be specifying this at the HTTP level (and standardize when using DIDComm over HTTP)?

I think ACA-Py's implementation shouldn't hang on to the http connection after a message has been processed, which means this wouldn't be an issue anymore.

For now I'll merge this as is as it's an improvement in itself. Let's keep looking at other options.

Agreed on both accounts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants