Fix duplex request proxying (especially gRPC) #80
Conversation
Fixing gRPC streaming at a glance: * We were not removing ASP .NET Core's limits on incoming request min data rate. That is a DoS protection mechanism that makes sense for normal http requests, but not for duplex or client-streaming channels. Note that there is precedent to doing what we are doing here. Both HTTP1 upgraded connections, as well as gRPC server middlewares shipped by Microsoft do the same for incoming requests within some criteria * We were not flushing bytes to HttpClient's machinery effectively. HttpClient keeps its own buffers on top of the underlying sockets, and we now properly flush bytes when reasonable to do so.
davidni
requested review from
Tratcher,
analogrelay and
halter73
as code owners
|
This PR is the result of several days of hunting down bugs with proxying gRPC comms. Discalimer: I have not been able to test this in this repo (or even build -- I don't have the preview .NET Core 5 SDK's) I ported changes I made in our internal implementation "by hand", so expect compile issues and possible runtime issues. @Tratcher would be great if you can help with those as my badnwidth is currently limited to contribute here. Thanks! |
Hmm, the build scripts should take care of this automatically. Did you use the instructions in the README to set up for building/testing in VS? I’d like to make sure the repo is accessible as possible for external contributors so feel free to file bugs for issues you encountered trying to build/test the repo. |
|
@anurse Thanks! I was worried that this would sully my dev machine, but I see that clearly you put in the effort to make this clean and easy, it worked great. Might be worthwhile making it even clearer in README that the restore script will NOT install anything globally and won't impair previous installations of .NET Core SDK, VS, etc. I pushed a new commit getting a green build + ported over unit tests. |
|
Oh yeah, we don’t want to go back to the world of special build shells or VMs to isolate different build environments :). Being well-isolated and safe for any contributor to use without messing with their machine is part of the design goals for the .NET Engineering System (Arcade) that we’re using here :). I’ll make a quick update pass over the README to clarify! |
davidni
changed the title
|
@davidni I think we've answered the hard question here and can proceed. Can you handle the cleanup feedback or would you like us to take over? |
…aming # Conflicts: # src/ReverseProxy.Core/Service/Proxy/HttpProxy.cs
|
@Tratcher done, I think I addressed all comments. |
|
Thanks |
6 participants
Fixes #79. At a glance: