aries-cloudagent-python
aries-cloudagent-python copied to clipboard
fix: return if return route but no response
Small fix to return an http inbound session if the return route decorator is present but the processing of the message did not elicit a response. Ended up being a quite straightforward fix. Current behaviour is to keep the socket open and hang indefinitely.
I would like some input on this PR as it changes behaviour and makes e.g. long polling impossible. However I do think there's ways to work around that. As @smithbk pointed out in #1372 we could reserve the noop
message from the pickup protocol for this.
Currently clients have to guess whether ACA-Py succesfully received the message as it won't return a 200 response. This mens that e.g. in AFJ we terminate the request after 15 seconds and just assume it went well if return routing was enabled.
We could of course disable return routing in the client (I'll make an additional PR to AFJ), but I think it's good to also fix the behaviour in ACA-Py.
The behaviour as implemented in this PR is also what's implemented in AFJ. We process the message, and if the processing didn't elicit a response we terminate the socket for HTTP. For websockets we keep it alive as it's more expected to have long lived sessions.
With current behaviour how it is implemented in AFJ's client side I was able to reduce the setup process (create connection with mediator, request mediation) from 17 to 1.7 seconds. Of course this is because AFJ has a set timeout of 15 seconds, but I suspect other implementations will have similar behaviour.
Fixes #1372
@andrewwhitehead -- can you please take a look at this one?
This PR essentially prevents "long-polling" for messages over http by sending a trustping with response_requested
false
and holding the socket open, if I'm not mistaken. We took advantage of this behavior from the toolbox at one point. It has since switched to using websockets. While long-polling would no longer be possible, I think it is acceptable to simply poll more regularly, if you must use http for implicit message pickup. It would be better to use websockets or an explicit pickup message (i.e. pickup protocol v2).
I've submitted a fix to @TimoGlastra's branch for the failing unit test
@TimoGlastra -- see the note above about the failing unit test. I think you need to do something here?
Ah I misunderstood. I thought @dbluhm pushed directly to my branch. I have merge the PR now
Codecov Report
Merging #1853 (bc084cb) into main (22ca606) will increase coverage by
0.00%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## main #1853 +/- ##
=======================================
Coverage 93.51% 93.51%
=======================================
Files 539 539
Lines 34600 34608 +8
=======================================
+ Hits 32355 32363 +8
Misses 2245 2245