-
-
Notifications
You must be signed in to change notification settings - Fork 573
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
[ReverseProxyPlugin] Fails to process request when response is too big #292
Comments
Hi @atsakiridis thank you for reporting this. We'll need to dig into it further, but |
Thanks for the insights @abhinavsingh. I will try to do some more digging in handler thread where receiving from upstream and sending back to client |
Thank you. I am currently traveling. I'll be able to get back to it in 2
weeks time. In meantime, feel free to share your findings, I can proceed
on-top of that.
…On Sat, Feb 15, 2020, 1:08 AM atsakiridis-test ***@***.***> wrote:
Thanks for the insights @abhinavsingh <https://github.com/abhinavsingh>.
I will try to do some more digging in handler thread where receiving from
upstream and sending back to client
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#292?email_source=notifications&email_token=AAA6Y4OMBH2TKVL3ERNJLPTRC6WKRA5CNFSM4KSTE5D2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEL3FWLY#issuecomment-586570543>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA6Y4JDZADEQXTRMIXTTTTRC6WKRANCNFSM4KSTE5DQ>
.
|
@abhinavsingh I think the culprit is here. If the full response isn't returned in the first call to Does this make sense? |
@atsakiridis That totally makes sense to me. I wrote this plugin mostly to demonstrate reverse proxy capabilities, definitely didn't thought about big enough responses requiring multiple recv. |
Cool. Let me do some research on how to best accommodate this functionality based on underlying architecture and we can discuss alternatives. By way, I also noticed in reverse_proxy.py that the connection to upstream is always cleartext so was thinking to add support for https, if that's what's used in the proxy pass url. If that makes sense for the project/community I could come up with a new enhancement issue on this and contribute a PR, so let me know. |
+1 to everything you suggested.
Few thoughts:
1) I have run into similar needs/use-case on another thread. Not exactly
related to reverse proxy plugin, but related to asynchronously read/write
from an upstream server.
2) Base problem I see, when asynchronous read/writes are needed from an
upstream server, plugin authors must handle asynchronous read/write within
plugin.
3) `proxy.py` core can help. By allowing plugin authors to register new
file descriptors for read/write events. Then, core can simply callback
plugin method when file descriptor is ready for read/writes, allowing for
really clean code in plugins.
4) `proxy.py` core already exposes such (file descriptor registration)
functionality. It is currently not exposed to plugins but should be
possible with little effort (I feel).
4) Totally agree on HTTPS use-case.
Thank you
------------------------------------------------------
Abhinav Singh,
https://abhinavsingh.com
------------------------------------------------------
…On Mon, Feb 17, 2020 at 12:09 AM Antonis Tsakiridis < ***@***.***> wrote:
Cool. Let me do some research on how to best accommodate this
functionality based on underlying architecture and we can discuss
alternatives.
By way, I also noticed in reverse_proxy.py that the connection to upstream
is always cleartext so was thinking to add support for https, if that's
what's used in the proxy pass url. If that makes sense for the
project/community I could come up with a new enhancement issue on this and
contribute a PR, so let me know.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#292?email_source=notifications&email_token=AAA6Y4JC3PCZYLZ5INBAQILRDGB7TA5CNFSM4KSTE5D2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEL4OVZA#issuecomment-586738404>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA6Y4N34M6MXHNM52NTUHDRDGB7TANCNFSM4KSTE5DQ>
.
|
Thanks for the insights 👍 let me do some digging there and I'll circle back with potential solutions. Let me also open up an issue for the https use case |
@abhinavsingh been doing some research on the codebase so let me share some ideas before I jump into coding to make sure we are on the same page. What we can do is:
Let me know if I'm missing something or something is off :) |
@abhinavsingh fyi I have forked the repo and started modeling a solution, so let me get back to you when I have news. |
👍👍👍🙂
…On Sun, Mar 8, 2020, 12:15 AM Antonis Tsakiridis ***@***.***> wrote:
@abhinavsingh <https://github.com/abhinavsingh> fyi I have forked the
repo and started modeling a solution, so let me get back to you when I have
news.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#292?email_source=notifications&email_token=AAA6Y4KIRXAJ24ZK4UNREK3RGKI3PA5CNFSM4KSTE5D2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOECRWI#issuecomment-596125913>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA6Y4PUP6PQNG3SHDBIPALRGKI3PANCNFSM4KSTE5DQ>
.
|
@abhinavsingh I created a draft PR for this that seems to be fixing the issue. I plan to iterate more on it to clean it up and test more comprehensively. Still would be nice if you can have a look and see if there are any gaps there or things that could be improved |
Hi @atsakiridis Thanks for the patience. These have been tough times with all the traveling involved. Good news, I am back at a stable place and will soon jump back onto your PR. Really appreciate it. 👍 |
No worries, the times are tough for everybody. Looking forward to it |
hi, @atsakiridis @abhinavsingh
the debug info for 3 times:
the last time if if len(tmp_response) < size, the interval is 5s every time, i don't know why. |
I think this approach can work with few modifications.
@atsakiridis This approach is the long term fix we want to go towards. As your research figured, most of the nut-bolts do exist but we need to tie them together. Yep, allowing plugins to register Thank you folks!!! |
Thank you for your reply. I have inspect the Content-Length response header, the problem is solved. |
@anthonykgross @widy28 Probably related, now there is a plugin |
Marking this as a feature request instead of bug. At this point, to fix this issue in a non-blocking fashion require re-thinking plugin interface to expose event loop. |
Hey @abhinavsingh, things are pretty crazy here so didn't get a chance to reply, sorry.
So yeah as I mentioned above I came up with a PR some months back that hooks on existing facilities to provide the (hopefully) needed functionality (I did some tests and the issue is no longer reproducible). Notice though it's not final yet, so was looking for your feedback on the PR to see if we need more adjustments but now I see it's closed, so wondering what happened :) |
@atsakiridis Agreed. Your PR #303 is a good start. However, I found it very specific to I closed the PR last week (left a comment on it) in view of:
I still want to get it addressed though. However, let's take a step at a time, instead of checking in a solution which only addresses Wdyt? |
I see your point @abhinavsingh, it's just too bad we couldn't sync at that time to agree on needed changes and get them out the door. Anyway, let me know if you have a plan to adapt this in a way that makes more general sense for the project and I'll be happy to help. Best regards |
Experienced similar problem for large data, only part of data could be received using ReverseProxyPlugin. conn.send(build_url)
data = bytearray()
while True:
package = conn.recv(DEFAULT_BUFFER_SIZE)
if len(package) < 1:
break
else:
data.extend(package)
self.client.queue(memoryview(data)) |
@jevy-wangfei @atsakiridis @widy28 Necessary support to proceed here has landed. Now plugins can too register for socket descriptor and read/write async. Proxy pool plugin has already been updated to use the same, see here #645
That should automatically solve several issues that we have experienced till now. Thanks for your patience and for being persistent :) |
@jevy-wangfei @atsakiridis @widy28 PTAL at #675 Thank you for your patience. |
Describe the bug
I try the reverse proxy example from README.md by starting proxy.py with (hopefully) appropriate flags (check below for invocation) and send simple requests using curl. For some reason proxy sporadically fails and times out after ~10 secs and client isn't receiving the response body. This happens around 20% of the times; the rest of them it's working fine.
To Reproduce
proxy.py
aspython -m proxy --disable-http-proxy --enable-web-server --plugins proxy.plugin.ReverseProxyPlugin --hostname 127.0.0.1
curl -v http://127.0.0.1:8899/get
. When reproducible I get:Notice the first 2 requests timing out at ~10 seconds and the 3rd going through fine.
Expected behavior
Was expecting to handle all requests without issues.
Version information
Additional context
Happy to help by debugging further & providing a patch for this (if indeed an issue), just need some pointers on where to look :)
Screenshots
N/A
The text was updated successfully, but these errors were encountered: