aries-cloudagent-python icon indicating copy to clipboard operation
aries-cloudagent-python copied to clipboard

fix: return if return route but no response

Open TimoGlastra opened this issue 2 years ago • 1 comments

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

TimoGlastra avatar Jul 04 '22 20:07 TimoGlastra

@andrewwhitehead -- can you please take a look at this one?

swcurran avatar Jul 14 '22 16:07 swcurran

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).

dbluhm avatar Nov 05 '22 20:11 dbluhm

I've submitted a fix to @TimoGlastra's branch for the failing unit test

dbluhm avatar Nov 05 '22 21:11 dbluhm

@TimoGlastra -- see the note above about the failing unit test. I think you need to do something here?

swcurran avatar Nov 08 '22 15:11 swcurran

Ah I misunderstood. I thought @dbluhm pushed directly to my branch. I have merge the PR now

TimoGlastra avatar Nov 08 '22 15:11 TimoGlastra

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

sonarcloud[bot] avatar Nov 08 '22 15:11 sonarcloud[bot]

Codecov Report

Merging #1853 (bc084cb) into main (22ca606) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1853   +/-   ##
=======================================
  Coverage   93.51%   93.51%           
=======================================
  Files         539      539           
  Lines       34600    34608    +8     
=======================================
+ Hits        32355    32363    +8     
  Misses       2245     2245           

codecov-commenter avatar Nov 08 '22 15:11 codecov-commenter