Three types of frames may be padded : DATA, HEADERS and PUSH_PROMISE.
Currently, each of these independently deals with padding and needs to
wait for and skip the initial padlen byte. Not only this complicates
frame processing, but it makes it very hard to process CONTINUATION
frames after a padded HEADERS frame, and makes it complicated to perform
atomic calls to h2s_decode_headers(), which are needed if we want to be
able to maintain the HPACK decompressor's context even when dropping
streams.
This patch takes a different approach : the padding is checked when
parsing the frame header, the padlen byte is waited for and parsed,
and the dpl value is updated with this padlen value. This will allow
the frame parsers to decide to overwrite the padding if needed when
merging adjacent frames.
Since commit f210191 ("BUG/MEDIUM: h2: don't accept new streams if
conn_streams are still in excess") we're refraining from reading input
frames if we've reached the limit of number of CS. The problem is that
it prevents such situations from working fine. The initial purpose was
in fact to prevent from reading new HEADERS frames when this happens,
and causes some occasional transfer hiccups and pauses with large
concurrencies.
Given that we now properly reject extraneous streams before checking
this value, we can be sure never to have too many streams, and that
any higher value is only caused by a scheduling reason and will go
down after the scheduler calls the code.
This fix must be backported to 1.9 and possibly to 1.8. It may be
tested using h2spec this way with an h2spec config :
while :; do
h2spec -o 5 -v -t -S -k -h 127.0.0.1 -p 4443 http2/5.1.2
done
We were returning a stream error of type PROTOCOL_ERROR on empty HEADERS
frames, but RFC7540#4.2 stipulates that we should instead return a
connection error of type FRAME_SIZE_ERROR.
This may be backported to 1.9 and 1.8 though it's unlikely to have any
real life effect.
These ones are not needed anymore since commit 97aaa67 ("MINOR: mux-h2:
only increase the connection window with the first update"). The tests
should now be more reliable. It might be worth simply removing all the
explicit handshake though it doesn't hurt and still serves as documentation.
Commit dc57236 ("BUG/MINOR: mux-h2: advertise a larger connection window
size") caused a WINDOW_UPDATE message to be sent early with the connection
to increase the connection's window size. It turns out that it causes some
minor trouble that need to be worked around :
- varnishtest cannot transparently cope with the WU frames during the
handshake, forcing all tests to explicitly declare the handshake
sequence ;
- some vtc scripts randomly fail if the WU frame is sent after another
expected response frame, adding uncertainty to some tests ;
- h2spec doesn't correctly identify these WU at the connection level
that it believes are the responses to some purposely erroneous frames
it sends, resulting in some errors being reported
None of these are a problem with real clients but they add some confusion
during troubleshooting.
Since the fix above was intended to increase the upload bandwidth, we
have another option which is to increase the window size with the first
WU frame sent for the connection. This way, no WU frame is sent until
one is really needed, and this first frame will adjust the window to
the maximum value. It will make the window increase slightly later, so
the client will experience the first round trip when uploading data,
but this should not be perceptible, and is not worth the extra hassle
needed to maintain our debugging abilities. As an extra bonus, a few
extra bytes are saved for each connection until the first attempt to
upload data.
This should possibly be backported to 1.9 and 1.8.
Released version 2.0-dev0 with the following main changes :
- BUG/MAJOR: connections: Close the connection before freeing it.
- REGTEST: Require the option LUA to run lua tests
- REGTEST: script: Process script arguments before everything else
- REGTEST: script: Evaluate the varnishtest command to allow quoted parameters
- REGTEST: script: Add the option --clean to remove previous log direcotries
- REGTEST: script: Add the option --debug to show logs on standard ouput
- REGTEST: script: Add the option --keep-logs to keep all log directories
- REGTEST: script: Add the option --use-htx to enable the HTX in regtests
- REGTEST: script: Print only errors in the results report
- REGTEST: Add option to use HTX prefixed by the macro 'no-htx'
- REGTEST: Make reg-tests target support argument.
- REGTEST: Fix a typo about barrier type.
- REGTEST: Be less Linux specific with a syslog regex.
- REGTEST: Missing enclosing quotes for ${tmpdir} macro.
- REGTEST: Exclude freebsd target for some reg tests.
- BUG/MEDIUM: h2: Don't forget to quit the sending_list if SUB_CALL_UNSUBSCRIBE.
- BUG/MEDIUM: mux-h2: Don't forget to quit the send list on error reports
- BUG/MEDIUM: dns: Don't prevent reading the last byte of the payload in dns_validate_response()
- BUG/MEDIUM: dns: overflowed dns name start position causing invalid dns error
- BUG/MINOR: compression/htx: Don't compress responses with unknown body length
- BUG/MINOR: compression/htx: Don't add the last block of data if it is empty
- MEDIUM: mux_h1: Implement h1_show_fd.
- REGTEST: script: Add support of alternatives in requited options list
- REGTEST: Add a basic test for the compression
- BUG/MEDIUM: mux-h2: don't needlessly wake up the demux on short frames
- REGTEST: A basic test for "http-buffer-request"
- BUG/MEDIUM: server: Also copy "check-sni" for server templates.
- MINOR: ssl: Add ssl_sock_set_alpn().
- MEDIUM: checks: Add check-alpn.
Add a way to configure the ALPN used by check, with a new "check-alpn"
keyword. By default, the checks will use the server ALPN, but it may not
be convenient, for instance because the server may use HTTP/2, while checks
are unable to do HTTP/2 yet.
In some situations, if too short a frame header is received, we may leave
h2_process_demux() waking up the task again without checking that we were
already subscribed.
In order to avoid this once for all, let's introduce an h2_restart_reading()
function which performs the control and calls the task up. This way we won't
needlessly wake the task up if it's already waiting for I/O.
Must be backported to 1.9.
It is now possible to specify a list of "alternatives" for a required
option. This must be done by separating options by a pipe ('|'). A test will be
executed if at least one of them is available. For instance:
#REQUIRED_OPTIONS=ZLIB|SLZ,LUA,OPENSSL
The function _findtest() has also been sligthly simplified.
In HTX, when the compression filter analyze the EOM, it flushes the compression
context and add the last block of compressed data. But, this block can be
empty. In this case, we must ignore it.
In dns_read_name() when dns name is used with compression and start position of
name is greater than 255 name read is incorrect and causes invalid dns error.
eg: 0xc11b c specifies name compression being used. 11b represent the start
position of name but currently we are using only 1b for start position.
This should be backported as far as 1.7.
A regression was introduced with efbbdf72 BUG: dns: Prevent out-of-bounds
read in dns_validate_dns_response() as it prevented from taking into account
the last byte of the payload. this patch aims at fixing it.
this must be backported in 1.8.
In mux_h2_unsubscribe, don't forget to leave the sending_list if
SUB_CALL_UNSUBSCRIBE was set. SUB_CALL_UNSUBSCRIBE means we were about
to be woken up for writing, unless the mux was too full to get more data.
If there's an unsubscribe call in the meanwhile, we should leave the list,
or we may be put back in the send_list.
This should be backported to 1.9.
With this patch we can provide a list of argument to reg-tests target.
Useful to run reg tests for a list of VTC files like that:
$ VARNISHTEST_PROGRAM=<...> make reg-tests reg-tests/checks/*.vtc
A check is done on HAProxy version to be sure it is supported. Internally, by
default, the macro 'no-htx' is set to "#". So in regtests, we can prefix
http-use-htx option line with this macro to inhibit the HTX. Concretly, this
line can be added in the HAProxy configuration:
${no-htx} option http-use-htx
When the option --use-htx is used, the macro 'no-htx' is unset.
By default a log directory is kept only if the test fails. With this option it
is possible to always keep it. If this option is used, the result of all tests
are displayed (and not only the failing ones).
Running regtests several times leaves many direcotries. It is painful to remove
them by hand. This command do it for you. It ask a confirmation to proceed to be
sure. The template used to create log direcotries has been changed. The prefix
'haregtests-' has been added to help the cleanup function to find existing log
direcotries.
Instead of directly executing varnishtest command, we use a variable to build
the command line and we execute it with the 'eval' builtin. This way it is
possible to have quoted parameters. For instance:
> ./scripts/run-regtests.sh --varnishtestparams "-Dmacro='some value' -n 10"
And the variable 'varnishtestparams' is also move at the end of the command
line, just before the list of test files. So it is possible to override all
default varnish options set by the script.
The help message is now in the function _help(). It exits the script with the
status 0 to not run regtests when the help is displayed. So it is also handy to
process script arguments before anything else. This avoids to start printing
messages related to regtests execution when it is not appropriate. Another
change, when it detects an invalid argument, the script exits with an error.
In si_release_endpoint(), if the end point is a connection, because we don't
know which mux to use it, make sure we close the connection before freeing it,
or else, we'd have a fd left for polling, which would point to a now free'd
connection.
This should be backported to 1.9.
Released version 1.9.0 with the following main changes :
- BUG/MEDIUM: compression: Use the right buffer pointers to compress input data
- BUG/MINOR: mux_pt: Set CS_FL_WANT_ROOM when count is zero in rcv_buf() callback
- BUG/MEDIUM: connection: Add a new CS_FL_ERR_PENDING flag to conn_streams.
- CONTRIB: debug: teach the "flags" utility about new conn_stream flags
- BUG/MEDIUM: stream-int: always clear CS_FL_WANT_ROOM before receiving
- BUG/MEDIUM: mux-h2: also restart demuxing when data are pending in demux
- BUG/MEDIUM: mux-h2: restart demuxing as soon as demux data are available
- BUG/MEDIUM: h2: fix aggregated cookie length computation in HTX mode
- MINOR: mux-h2: report more h2c, last h2s and cs information on "show fd"
- CONTRIB: debug: report stream-int's flag SI_FL_CLEAN_ABRT
- MINOR: cli/stream: add the conn_stream in "show sess" output
- BUG/MINOR: mux-h2: don't report a fantom h2s in "show fd"
- BUG/MINOR: cli/fd: don't isolate the thread for each individual fd
- MINOR: objtype: report a few missing types in names and base pointers
- BUG/MEDIUM: mux-h2: make sure to report synchronous errors after EOS
- BUG/MEDIUM: mux-h2: report asynchronous errors in h2_wake_some_streams()
- BUG/MEDIUM: mux-h2: make sure the demux also wakes streams up on errors
- BUG/MINOR: mux-h1: report the correct frontend in error captures
- BUG/MEDIUM: stream-int: also wake the stream up on end of transfer
- MEDIUM: h2: properly check and deduplicate the content-length header in HTX
- BUG/MEDIUM: stream: Forward the right amount of data before infinite forwarding
- BUG/MINOR: proto_htx: Call the HTX version of the function managing client cookies
- BUG/MEDIUM: lua/htx: Handle EOM in receive/get_line calls in HTTP applets
- BUG/MINOR: lua: Return an error if a legacy HTTP applet doesn't send anything
- MINOR: compression: Remove the thread_local variable buf_output
- CLEANUP: connection: rename subscription events values and event field
- CLEANUP: connection: rename conn->mux_ctx to conn->ctx
- MINOR: connection: remove an unwelcome dependency on struct stream
- CLEANUP: stream-int: consistently call the si/stream_int functions
- BUG/MEDIUM: h1: Don't shutw/shutr the connection if we have keepalive.
- BUG/MEDIUM: H2: Make sure htx is set even on empty frames.
- BUG/MEDIUM: mux-h2: pass CS_FL_ERR_PENDING to h2_wake_some_streams()
- MEDIUM: stream-int: always consider all CS errors on the send side
- BUG/MEDIUM: h2: Make sure we don't set CS_FL_ERROR if there's still data.
- CLEANUP: mux-h2: implement h2s_notify_{send,recv} to report events to subscribers
- MINOR: mux-h2: add a new function h2s_alert() to call the data layer
- BUG/MEDIUM: mux-h2: make use of h2s_alert() to report aborts
- MINOR: connection: add cs_set_error() to set the error bits
- CLEANUP: mux-h2: make use of cs_set_error()
- BUG/MINOR: mux-h2: make sure we check the conn_stream in early data
- BUG/MEDIUM: h2: Don't wait for flow control if the connection had a shutr.
- MINOR: cli/show_fd: report that a connection is back or not
- SCRIPTS: add the slack channel URL to the announce script
- CLEANUP: remove my name and address from the copyright banner
- DOC: mention in the readme that 1.9 is a stable version now
First, it's a pain to always have to think about updating this date,
second for a long time I've not been the only developer there, and third,
some users contact me hoping to get help that I can't deliver. It's about
time to redirect them to the main site where all the useful links should
be.
In h2_snd_buf(), if we couldn't send the data because of flow control, and
the connection got a shutr, then add CS_FL_ERROR (or CS_FL_ERR_PENDING). We
will never get any window update, so we will never be unlocked, anyway.
No backport is needed.
When dealing with early data we scan the list of stream to notify them.
We're not supposed to have h2s->cs == NULL here but it doesn't cost much
to make the scan more robust and verify it before notifying.
No backport is needed.
If we had no pending read, it could be complicated to report an
RST_STREAM to a sender since we used to only report it via the
rx side if subscribed. Similarly in h2_wake_some_streams() we
now try all methods, hoping to catch all possible events.
No backport is needed.
In order to report an error to the data layer, we have different ways
depending on the situation. At a lot of places it's open-coded and not
always correct. Let's create a new function h2s_alert() to handle this
task. It tries to wake on recv() first, then on send(), then using
wake().
In the mux h2, make sure we set CS_FL_ERR_PENDING and wake the recv task,
instead of setting CS_FL_ERROR, if CS_FL_EOS is not set, so if there's
potentially still some data to be sent.
We still have an issue with asynchronous errors, which is that while
they don't truncate reads anymore, they might be missed during a
send() attempt. This can happen for example when processing a request
followed by undesired data for which the stream doesn't try to receive,
while the send side experiences an error (transfer aborted by the client).
In this case we definitely want all send() attempts to fail as soon as
the error was reported, even if it's only pending. This way we leave an
opportunity to the stream interface to try to receive the last data
pending in the buffer but it cannot send anymore and knows that there
is an error when trying to do so.
Commiy 8519357c ("BUG/MEDIUM: mux-h2: report asynchronous errors in
h2_wake_some_streams()") addressed an issue with synchronous errors
but forgot to fix the call places to also pass CS_FL_ERR_PENDING
instead of CS_FL_ERROR.
No backport is needed.
