Hello!

On Tue, Jul 25, 2017 at 06:28:16PM -0700, Piotr Sikora wrote:

> Hey Maxim,
>
> > 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.
>
> I strongly disagree. Changes introduced to ngx_http_v2.c and
> ngx_http_v2_filter_module.c are minimal.
>
> Also, having separate client-side and server-side code would result in
> thousands of lines of duplicated code, for no reason, really.

Biggest concert here are various "if (h2c->server) { ... } else {
.... }" constructions. Certainly there can (and probably should)
be some degree of code reuse between client and server code,
though it would be much better to keep them easily separateable.

> > 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;
>
> But that's basically:
>
> cat ngx_http_proxy_module.c \
> | sed 's/ngx_http_proxy/ngx_http_proxy_v2/g' \
> > ngx_http_proxy_v2_module.c
>
> I don't see how copying ~4500 lines of code is a good idea.

This is how our upstream protocol modules are currently
implemented: each module provides a configuration-specific parts,
as well as it's protocol-specific parts. Reducing duplication of
configuration parsing is one of the main reasons of the "all
protocols in the proxy module" plan as described above. This is
not what we currently have though, and introducing ad-hock hacks
to save some lines of code looks much worse than some amount of
code duplication.

Also, keeping the module separate would actually allow to reduce
amount of code needed in some cases. For example, since there is no
cache support, at least in initial implementation, it would be
possible to simply omit relevant directives.

> Also, as mentioned elsewhere, one of the reasons for adding HTTP/2
> code to the proxy module was the ability to negotiate HTTP/1.1 or
> HTTP/2 via ALPN. Creating a separate HTTP/2-to-upstreams module won't
> allow to add such feature in the future.

Such feature is anyway does not look compatible with current
upstream infrastructure, as request is created once, before even
connecting to backend servers, and it is not possible to switch to
a different protocol after a connection.

> > - start working on the different protocols in the proxy module,
> > and introduce HTTP/2 proxying after this work is done.
>
> How is that even an option here? It's going to take forever before
> such rewrite is done, and I have no desire nor time to work on that.

Well, if you insist on doing things in the proxy module, posponing
HTTP/2 till the rewrite is done is probably the only option. We
can consider prioritizing the rewrite rewrite accordingly.

[...]

> > - 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.
>
> I agree that ngx_http_v2_init_connection() isn't perfect, however,
> fake connections are an abstraction layer that needs to be added
> somewhere. The same is done for SSL, and it's somehow acceptable.
>
> I'm happy to use whatever generic mechanism is available, but there is
> none at the moment, and I don't see how adding even more code and
> complexity to this already big patchset is going to get us anywhere.

The problem here is that HTTP/2 is not a separate layer, in
contrast to SSL, but rather tries to be an application-level
protocol. On the other hand, this is probably the problem of
HTTP/2 protocol itself, and we have to deal with it somehow.

> > - 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.
>
> Except that HTTP/2 headers as read from the wire, even if parsed in
> separate module, couldn't be simply written to a cache file, because
> of stateful HPACK encoding, so serialization would need to be done
> anyway.

When needed, it may better idea to keep serialization within the
HTTP/2 protocol module instead, similarly to how we skip large
stderr in FastCGI.

> Anyway, it appears that your "serious concerns" are mostly about
> organization of the code, and not the code itself. What's unclear to
> me is how much of the code review this patchset actually received,
> i.e. if the existing code would be moved to a separate
> HTTP/2-to-upstreams module, would it be acceptable or do you have
> other issues with the code?

As of now, the code doesn't not look committable even from
architecture point of view. It is yet to recieve further review
if/when architectural issues are resolved.

--
Maxim Dounin
http://nginx.org/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel