Hello! On Thu, Jun 22, 2017 at 01:33:16PM -0700, Piotr Sikora via nginx-devel wrote:
> # HG changeset patch > # User Piotr Sikora <piotrsik...@google.com> > # Date 1490769087 25200 > # Tue Mar 28 23:31:27 2017 -0700 > # Node ID 7eb807b056da7abe9c679b59e94595d59a1406e6 > # Parent 0637acdb51e29e1f68f5f3e762115c702cab4e72 > Proxy: add HTTP/2 support. > > Signed-off-by: Piotr Sikora <piotrsik...@google.com> [...] There are serious concerns about this and with other patches. Most generic ones are outlined below. 1. Server-side HTTP/2 code complexity concerns. As per discussion with Valentin, introducing client-related code paths in the server HTTP/2 code seems to complicate things. Given that the complexity of the code is already high, it might be better idea to implement separate client-side processing instead. 2. Different protocols in proxy module. You've introduced a completely different protocol in the proxy module, which contradicts the established practice of using separate modules for different protocols. Reasons for having all (or at least may of) the protocols available in the proxy module instead are well understood, and there is a long-term plan to revise current practice. The plan is to preserve protocol-specific modules as a separate entities, but let them share the common configuration directives, similar to how it is currently done with upstream{} blocks and the overall upstream infrastructure. So one can write something like proxy_pass fastcgi://127.0.0.1:9000; and get an expected result. While benefits of having all protocols sharing the same user-visible interface are understood, approach taken with HTTP/2 implementation is considered suboptimal, and will likely lead to something hard to maintain. I see two possible solutions here: - make HTTP/2-to-upstreams a separate full-featured upstream-based module, similar to proxy or fastcgi; - start working on the different protocols in the proxy module, and introduce HTTP/2 proxying after this work is done. Additionally, there are also some minor / related comments: - Parts of the code, tightly coupled with upstream infrastructure, notably ngx_http_v2_upstream_output_filter(), are placed into v2/ directory. Instead, they seem to belong to the HTTP/2-to-upstream module implementation, as suggested in (1). - Upstream infrastructure as available in src/http/ngx_http_upstream.c is expected to be protocol-agnostic. Introducing calls like ngx_http_v2_init_connection() there is a layering violation. Instead, there should be something more generic. - Doing protocol parsing elsewhere instead of parsing things based on what nginx got from the connection results in some generic upstream mechanisms rendered not working - notably, it is no longer possible to simply write headers to a cache file. Serialization introduced instead, at least in its current form, looks more like a bandaid. -- Maxim Dounin http://nginx.org/ _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel