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.

> 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.

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.

> - 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.

> 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).

Sure, this and a few other functions could be added to different files.

> - 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.

> - 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.

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?

Best regards,
Piotr Sikora
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel