H2 related corrections and optimizations.

[aleksostapenko]

Bugs

• The header 'age' is added unconditionally here; thus, looks like it should be skipped in tfw_cache_copy_resp();
  MekhanikEvgenii - will be fixed in Fix double adding 'age' header. #2398

• data = tfw_pool_alloc_not_align(it->pool, sz) should be moved down - after the static index processing;
  MekhanikEvgenii - Fixed by Alloc memory in tfw_hpack_hdr_name_set only for dynamic indexing #2350

• Header 'host' can have empty field-value here (this possibility is mentioned in RFC 7230 section 5.4); (Outdated code. Host header can't be empty for http2. http2_general.test_h2_headers contains test_empty_host_header that verifies this.)

• Due to the nature of the internal implementation of encoder dynamic table in TempestaFW (for performance purposes), its maximum size is limited - it cannot be greater than HPACK_ENC_TABLE_MAX_SIZE; so for now, if client sends SETTINGS frame with new max window size greater than HPACK_ENC_TABLE_MAX_SIZE, the table inconsistency between endpoints may arise; to avoid this problem - the WinodwUpdate HEADERS frame with HPACK_ENC_TABLE_MAX_SIZE should be sent to client's decoder (within the first following HEADERS frame of the following SETTINGS acknowledgment, see RFC 7541 section 4.2); This is SETTINGS_HEADER_TABLE_SIZE > 4096 bytes does not work as expected #1807

• The condition here should be

if (hclosed_streams->num <= TFW_MAX_CLOSED_STREAMS
   && (max_streams > ctx->streams_num
      || !hclosed_streams->num))
{
    . . .
}

MekhanikEvgenii - Fixed by #2351

• Duplicated cookie headers should be concatenated before forwarding to HTTP/1.1 connection into one singular header (RFC 7540 section 8.1.2 p. 5 "Compressing the Cookie Header Field"); Forwarding http2 splitted cookie header for #1736

• It seems like a bug here, since the function is exiting in any case and the remainder of the eolen in the next fragment is not deleted; besides, tfw_http_msg_del_eol() function is completely unused for now, as well as skb_next_data(), thus looks like these procedures can be removed;
  MekhanikEvgenii - This function is already removed.

• (TBD) For now TempestaFW closes connection in case of any errors during parsing the message, but in case of HTTP/2 connection the malformed messages should be treated as a stream error and not the connection-wide error, which means that connection should not be closed - only stream (RFC7540 section 8.1.2 p. 6 "Malformed Requests and Responses");
  MekhanikEvgenii - We decide that we can't do it because client add all sent headers to it's hpack dynamic table. When we can't parse message, we don't save headers in hpack dynamic table, moreover we discard remaining message data after error occurs! So we can't to serve new requests from such client.

• Looks like assignment should be moved to TDBv0.2: Cache background revalidation and eviction #515

(*r)->len = curr_ptr - (*r)->data

• Currently, the stream pointer is reset to avoid double calling of stream FSM if error occurred during response generation; this solution works for cases where possible tfw_h2_stream_id_close() calls are close to each other (e.g in tfw_h2_resp_adjust_fwd() procedure) and stream_id can be saved locally between calls; but this way is not suitable for the cases where tfw_h2_stream_id_close() calls are rather far apart - the example of such case is failed redirection response generating: if tfw_h2_stream_id_close() had been called in tfw_h2_prep_redirect(), but then the error occurred before the response is actually created/sent - the tfw_h2_send_resp() will not send error response to the client, since req->stream is already NULL; to resolve the problem in general way, the tfw_h2_stream_id_close() function should be changed a little, to assign id to the special new internal field of req->pit (instead of returning it as now) and check it during subsequent processing (i.e. if the error response should be generated after failed forwarding response, or failed redirection response generating) - before calling tfw_h2_stream_id_close(); if this field is set (non-zero) then tfw_h2_stream_id_close() had already been called and it is needn't to be called again - just use the id saved in the field; if field is not set (zero) - then tfw_h2_stream_id_close() should be called; this field need not be synchronized, since it can be accessed only from one thread (unlikely the req->stream field) in which current request is processed now (during forwarding request to server, or forwarding corresponding response to client); this approach should allow to completely avoid the problem with multiple calling to tfw_h2_stream_id_close() during internal responses generation (including failed redirection response generating) and during server response forwarding.
  MekhanikEvgenii - This problem was fixed in several different PRs. First of all this part of code was reworked during moving making frames to xmit callback and during moving stream processing (send part) to the xmit callback also. Finally problem was fixed in da24b86

Optimizations

• It seems that just one call of tfw_http_hdr_split() procedure can be used here instead of tfw_http_msg_srvhdr_val() call and tfw_h2_msg_hdr_length() call below (i.e. for now @h_val and @s_val are two variables for the same instance);
  MekhanikEvgenii - Will be fixed by Remove unnecessary tfw_http_hdr_split #2368. It seems that we can remove tfw_http_hdr_split instead of tfw_http_msg_srvhdr_val

• In case of response forwarding (TFW_H2_TRANS_INPLACE type of H2 encoding operations) the tfw_http_hdr_split() procedure is called twice for the same header: here and here; maybe it is worth to call this procedure only once, but earlier - in the beginning of @tfw_hpack_encode() function, before the tfw_hpack_encoder_index() procedure call. See also Rewrite tfw_hpack_node_compare to make it clean & fast #1920 (comment) and Rewrite tfw_hpack_node_compare to make it clean & fast #1920 (comment)
  MekhanikEvgenii - fixed in Call tfw_http_hdr_split only during hpack encoding  #2367

• In case of Huffman-encoded header name and/or value - it is worth to allocate (and assign to it->rspace) a bit more size of memory here; e.g. as mentioned in Exploring HTTP/2 Header Compression section 3.2 - Huffman can compress by 20% on average, which means that allocation about 125% of initial Huffman-encoded size (i.e. res_len = len + len/4) will help to avoid in most cases additional space allocations and TfwStr expansions in tfw_hpack_huffman_write() procedure (note, that in this case the warnings for it->rspace here and in other similar places should be removed);
  MekhanikEvgenii - will be fixed in Mekhanik evgenii/1411 7 #2403

• If tfw_hpack_exp_hdr() function is called here, then the intersection with it->parsed_hdr descriptor allocations in req->pool is occurred, and the following parsing of the header's value will require the full re-allocations in req->pool (instead of fast path) in tfw_pool_realloc() procedure; this problem can be avoided either via the another pool addition (specially for it->hdr descriptors), or via the re-initialization of it->hdr descriptor; in the latter variant the it->hdr will be reinitialized in any case here - with new values of it->pos and length (like in BUFFER_NAME_OPEN() macro), and old descriptors(s) in it->hdr for header's name will be just forgotten (as they are not needed any more, since the name is parsed by now, and all necessary information is already in it->parsed_hdr); note however, that in this variant the intersection with it->parsed_hdr descriptor allocations in req->pool can still take place if it->hdr became compound during Huffman decoding of header's value, but considering changes proposed in p.3 above - the case of tfw_hpack_exp_hdr() calling from tfw_hpack_huffman_write() should be very rare, thus in most cases it->hdr should remain plain and intersections will not occur;
  MekhanikEvgenii - will be fixed in Mekhanik evgenii/1411 7 #2403 and Remove unnecessary call of tfw_hpack_exp_hdr. #2447

• For now TempestaFW does not know static indexes for headers which are defined in configuration file for adding, substituting or appending, but this headers are fully known on configuration stage, so it is worth try to determine static indexes for them; looks like two ways are possible: either to allow the specification of static indexes for corresponding headers directly in the configuration file, or to implement something like tag search in Exploring HTTP/2 Header Compression section 3.1 (in part devoted to headers defined in static table) - on configuration stage. Done in Fixed framing error for resposnses received from cache with header modification #1831.

• Since tfw_h2_stream_id() function is called only from request receive path, and there must not be concurrent access to req->stream until request will be passed to the server connection - locking in tfw_h2_stream_id() can be removed;
  MekhanikEvgenii - there is a PR Mekhanik evgenii/1411 3 #2365, which should fix this problem. In fact this point is not correct: when cache is not shard we don't need to call it at all, because stream_id can't be 0 here. If cache is shard there can be concurrent access to req->stream, because cache processing can be called on other cpu. But there is no sence to check stream_id also, because if stream can be closed in any time! For example if stream_id is not equal to zero before tfw_cache_build_resp there is no guarantee that it will not became zero during this function call! We check that req->stream is not NULL during calling tfw_h2_stream_init_for_xmit when we access req->stream pointer.

• Perhaps it would be wise to create the version of tfw_h2_stream_id_close() procedure for cases when it is called from request receive path (e.g. from request processing in cache - here and here), since there must not be concurrent access to req->stream, and it must be valid (non-NULL) until request will be passed to the server connection.
  MekhanikEvgenii - there is a PR Mekhanik evgenii/1411 3 #2365, which should fix this problem. In fact this point is not correct: when cache is not shard we don't need to call it at all, because stream_id can't be 0 here. If cache is shard there can be concurrent access to req->stream, because cache processing can be called on other cpu. But there is no sence to check stream_id also, because if stream can be closed in any time! For example if stream_id is not equal to zero before tfw_cache_build_resp there is no guarantee that it will not became zero during this function call! We check that req->stream is not NULL during calling tfw_h2_stream_init_for_xmit when we access req->stream pointer.

[krizhanovsky]

The current HTTP/2 implementation and TODO issues must be revised according to the RFC 9113.