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: return if return route but no response #1853

Conversation

TimoGlastra
Copy link
Contributor

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

@swcurran
Copy link
Contributor

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

@dbluhm
Copy link
Contributor

dbluhm commented Nov 5, 2022

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
dbluhm previously approved these changes Nov 5, 2022
@dbluhm
Copy link
Contributor

dbluhm commented Nov 5, 2022

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

@swcurran
Copy link
Contributor

swcurran commented Nov 8, 2022

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

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
@TimoGlastra
Copy link
Contributor Author

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

@sonarcloud
Copy link

sonarcloud bot commented Nov 8, 2022

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

@codecov-commenter
Copy link

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           

Copy link
Contributor

@swcurran swcurran left a comment

Choose a reason for hiding this comment

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

Approved based on @dbluhm 's approval and fixed integration test.

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.

HTTP request hangs with return_route header
4 participants