WIP: HTTPDecoder: Reenable Parsing After Failed Upgrade

[jshier]

Reenable parsing in HTTPDecoder after an upgrade is attempted but failed by the response.

Motivation:

Currently, HTTPDecoder sets stopParsing = true whenever a message is parsed and an upgrade is detected (isUpgrade == true). Unfortunately, this means that if the upgrade wasn't successful, the decoder silently blocks any attempt to reuse the connection, essentially hanging the client until they timeout.

Modifications:

I've attempted to generalize HTTPDecoder's conformance to WriteObservingByteToMessageDecoder so that it can reenable parsing based on the outgoing HTTPServerResponsePart.

Result:

A properly reusable decoder.

---

WIP:

I need a few things to finish this PR.

• Proper way to generalize the write(data:) implementation. Right now I just check the decoder's full metatype. What's the preferred pattern here?
• Proper condition for reenabling parsing. Right now I ensure that the connection will be kept alive (if it's being killed who cares), that the response is NOT an upgrade, and that it has stopped parsing.
• Proper test. I see the HTTPDecoderTest but it's not clear what kind of test would be best here, or how to simulate this scenario exactly.

Thinking about this more, how can I ensure that the outgoing response is for the incoming request?

[jshier]

I believe I've verified that my fix works (local server can be used to pass the Alamofire test that previously hung), but I still need guidance around the proper conditions and unit test.

[Lukasa]

Proper way to generalize the write(data:) implementation. Right now I just check the decoder's full metatype. What's the preferred pattern here?

You can use HTTPDecoder.kind, which tells you which kind of decoder you are.

Proper condition for reenabling parsing. Right now I ensure that the connection will be kept alive (if it's being killed who cares), that the response is NOT an upgrade, and that it has stopped parsing.

I think for now this is appropriate.

Proper test. I see the HTTPDecoderTest but it's not clear what kind of test would be best here, or how to simulate this scenario exactly.

I think you want a few scenarios:

1. Send an upgrade request, do not accept the upgrade, and then send another request. Confirm both get through.
2. Pipeline a pair of requests. This is generally unsound and we don't really support it, but if the upgrade fails then we should resume parsing the second one. (I have an inkling this test will fail, it's possible that we need to trigger ourselves to start parsing again in this case which will be a fun wrinkle).

To achieve "do not accept the upgrade" you don't actually need an encoder, you can just write the response head into the pipeline. EmbeddedChannel doesn't care what data you pass through it.

[weissi]

@swift-nio-bot test performance please

[weissi]

@swift-nio-bot performance test please

[Lukasa]

@swift-server-bot test performance please

[Lukasa]

@swift-server-bot add to allowlist

[Lukasa]

@swift-server-bot test perf please

[swift-server-bot]

performance report

build id: 151

timestamp: Thu Oct 26 07:07:05 UTC 2023

results

name	min	max	mean	std
write_http_headers	0.042764887	0.04303109	0.0428531476	9.165968117650048e-05
http_headers_canonical_form	0.105976048	0.106613896	0.1063023122	0.0002636995557017283
http_headers_canonical_form_trimming_whitespace	0.020746165	0.021389884	0.020859861	0.00019799952969416676
http_headers_canonical_form_trimming_whitespace_from_short_string	0.018789331	0.019371804	0.0188702018	0.00017759802584225594
http_headers_canonical_form_trimming_whitespace_from_long_string	0.030208525	0.030739893	0.030288334	0.00016029743227721012
bytebuffer_write_12MB_short_string_literals	0.142562903	0.150421367	0.1441933777	0.0024653019083271215
bytebuffer_write_12MB_short_calculated_strings	0.069466214	0.070176513	0.0697632713	0.00023521549303660893
bytebuffer_write_12MB_medium_string_literals	0.942097862	0.954554064	0.9457006427	0.004845470244392264
bytebuffer_write_12MB_medium_calculated_strings	0.086782952	0.087900492	0.0872593308	0.00038563125837509107
bytebuffer_write_12MB_large_calculated_strings	0.164453123	0.171150711	0.1660514606	0.0021527158760489954
bytebuffer_lots_of_rw	0.043117463	0.043766891	0.0433166383	0.0002385047075915784
bytebuffer_write_http_response_ascii_only_as_string	0.029871606	0.030537487	0.0301285857	0.0002392813635311876
bytebuffer_write_http_response_ascii_only_as_staticstring	0.029911716	0.030837597	0.030139898	0.00029808828077638723
bytebuffer_write_http_response_some_nonascii_as_string	0.029177535	0.030007743	0.029561930200000003	0.00021286835340745202
bytebuffer_write_http_response_some_nonascii_as_staticstring	0.02990656	0.030538495	0.0300881944	0.00022453140378881918
no-net_http1_1k_reqs_1_conn	0.013450951	0.013892555	0.013539905299999998	0.00012764605954839852
http1_1k_reqs_1_conn	0.062408803	0.064217022	0.06353708890000001	0.0005167480023752074
http1_1k_reqs_100_conns	0.093203476	0.093545682	0.0933346667	0.00012548584404275357
future_whenallsucceed_100k_immediately_succeeded_off_loop	0.082102631	0.083262915	0.0827453198	0.00036438972741430133
future_whenallsucceed_100k_immediately_succeeded_on_loop	0.08210656	0.089647005	0.08357218990000001	0.0024096257159989926
future_whenallsucceed_10k_deferred_off_loop	0.023270075	0.023741969	0.0234589797	0.00012911069295079079
future_whenallsucceed_10k_deferred_on_loop	0.014501764	0.015091909	0.014653873100000001	0.00016686902421257104
future_whenallcomplete_100k_immediately_succeeded_off_loop	0.03932471	0.040086276	0.0395638353	0.00023733492101577953
future_whenallcomplete_100k_immediately_succeeded_on_loop	0.039345693	0.040008557	0.0396448797	0.0002558951073393808
future_whenallcomplete_10k_deferred_off_loop	0.015512314	0.016931841	0.0157437644	0.00043880381093837866
future_whenallcomplete_100k_deferred_on_loop	0.079030741	0.082391037	0.0799885964	0.0009150300408086431
future_reduce_10k_futures	0.016651313	0.017178917	0.0167741484	0.00015034891046436646
future_reduce_into_10k_futures	0.014419065	0.014954547	0.014535257099999999	0.00015961771494936146
channel_pipeline_1m_events	0.097174262	0.097352843	0.0972686097	7.510920539831719e-05
websocket_encode_50b_space_at_front_100k_frames_cow	0.049522627	0.049984065	0.0496394245	0.000182097347845261
websocket_encode_50b_space_at_front_1m_frames_cow_masking	0.673983686	0.676064011	0.6744788783	0.0006336520574896763
websocket_encode_1kb_space_at_front_1m_frames_cow	0.521021385	0.526284634	0.521844222	0.0015748137188687208
websocket_encode_50b_no_space_at_front_100k_frames_cow	0.049871428	0.05034093	0.050027162599999994	0.00020003531289316528
websocket_encode_1kb_no_space_at_front_100k_frames_cow	0.052049898	0.052502451	0.0521691679	0.00017871766257327644
websocket_encode_50b_space_at_front_100k_frames	0.075648943	0.076155042	0.07589705620000001	0.00019900313449055748
websocket_encode_50b_space_at_front_10k_frames_masking	0.009278494	0.009372676	0.009314418199999999	2.9643237065700757e-05
websocket_encode_1kb_space_at_front_10k_frames	0.012574961	0.013036623	0.0126361062	0.00014157926995605303
websocket_encode_50b_no_space_at_front_100k_frames	0.075524543	0.075962202	0.0757605938	0.00015762248689602984
websocket_encode_1kb_no_space_at_front_10k_frames	0.011946892	0.012413755	0.012005771600000001	0.00014409646068226506
websocket_decode_125b_10k_frames	0.012627712	0.012750931	0.012700226700000001	4.110339529649603e-05
websocket_decode_125b_with_a_masking_key_10k_frames	0.013048638	0.013604126	0.0131429248	0.00016326155984241274
websocket_decode_64kb_10k_frames	0.01295203	0.013702083	0.0132177657	0.00020867119829959501
websocket_decode_64kb_with_a_masking_key_10k_frames	0.013356652	0.013922202	0.0135015137	0.0001598330899970688
websocket_decode_64kb_+1_10k_frames	0.0129766	0.013314445	0.013100153	0.00010378001586903781
websocket_decode_64kb_+1_with_a_masking_key_10k_frames	0.013347262	0.013858828	0.013524460999999998	0.00015087382199484875
circular_buffer_into_byte_buffer_1kb	0.033005886	0.033491951	0.0331123763	0.00019769198985180645
circular_buffer_into_byte_buffer_1mb	0.064681011	0.065172406	0.0648893406	0.00023006667245252448
byte_buffer_view_iterator_1mb	0.017561255	0.018045793	0.0176269416	0.00014764884398629392
byte_buffer_view_contains_12mb	0.052877447	0.053377746	0.0530549114	0.00022260439986676904
byte_to_message_decoder_decode_many_small	0.041698072	0.051183989	0.0427617859	0.0029652680720491896
generate_10k_random_request_keys	0.090287404	0.090641042	0.09048328159999999	0.0001406432132163597
bytebuffer_rw_10_uint32s	0.041089399	0.041974817	0.0413744957	0.0002768007682291236
bytebuffer_multi_rw_10_uint32s	0.073854486	0.074583994	0.07411126539999999	0.0002575338503395467
lock_1_thread_10M_ops	0.151240614	0.152511946	0.1518350162	0.00043253013912502414
lock_2_threads_10M_ops	0.834906333	0.873009558	0.859538105	0.010815858664317446
lock_4_threads_10M_ops	0.891242095	0.922027725	0.9110281466	0.010356854211792364
lock_8_threads_10M_ops	0.975776571	1.001712303	0.9926601068	0.007307278964689411
schedule_100k_tasks	0.061631529	0.105347832	0.07140298149999999	0.013251139094085856
schedule_and_run_100k_tasks	0.237549448	0.24272745	0.24025425660000002	0.0017043206631627802
execute_100k_tasks	0.100502367	0.103009813	0.10148698910000001	0.000939171523579688
bytebufferview_copy_to_array_100k_times_1kb	0.011501877	0.011606393	0.0115388097	2.9276774560695146e-05
circularbuffer_copy_to_array_10k_times_1kb	0.019785245	0.020218636	0.0198429979	0.0001331699842544196
deadline_now_1M_times	0.024648632	0.024915164	0.0247650138	9.137735403722537e-05
asyncwriter_single_writes_1M_times	0.598652287	0.598983178	0.5988035882	0.0001110879092728996
asyncsequenceproducer_consume_1M_times	0.826901793	0.827442197	0.8270436293	0.00015739476453953657
udp_10k_writes	0.37734095	0.381838999	0.3781240473	0.0013353878890657851
udp_10k_vector_writes	0.204988632	0.205624407	0.20529033179999998	0.00020763409779267158
udp_10k_vector_reads	0.386314055	0.387382396	0.38665071819999997	0.00036255459063576774
udp_10k_vector_reads_and_writes	0.108990303	0.109571306	0.10926119190000001	0.0001963570755607398
tcp_100k_messages_throughput	0.902313439	0.914702897	0.9081547792	0.004003094093935723

comparison

name	current	previous	winner	diff
write_http_headers	0.042764887	0.041215528	previous	3%
http_headers_canonical_form	0.105976048	0.106348938	current	0%
http_headers_canonical_form_trimming_whitespace	0.020746165	0.020946744	current	0%
http_headers_canonical_form_trimming_whitespace_from_short_string	0.018789331	0.01902425	current	-1%
http_headers_canonical_form_trimming_whitespace_from_long_string	0.030208525	0.03082749	current	-2%
bytebuffer_write_12MB_short_string_literals	0.142562903	0.142559886	previous	0%
bytebuffer_write_12MB_short_calculated_strings	0.069466214	0.071344047	current	-2%
bytebuffer_write_12MB_medium_string_literals	0.942097862	0.935958429	previous	0%
bytebuffer_write_12MB_medium_calculated_strings	0.086782952	0.086752859	previous	0%
bytebuffer_write_12MB_large_calculated_strings	0.164453123	0.165660809	current	0%
bytebuffer_lots_of_rw	0.043117463	0.042981179	previous	0%
bytebuffer_write_http_response_ascii_only_as_string	0.029871606	0.028030235	previous	6%
bytebuffer_write_http_response_ascii_only_as_staticstring	0.029911716	0.028173718	previous	6%
bytebuffer_write_http_response_some_nonascii_as_string	0.029177535	0.028314961	previous	3%
bytebuffer_write_http_response_some_nonascii_as_staticstring	0.02990656	0.028269317	previous	5%
no-net_http1_1k_reqs_1_conn	0.013450951	0.011678896	previous	15%
http1_1k_reqs_1_conn	0.062408803	0.060070308	previous	3%
http1_1k_reqs_100_conns	0.093203476	0.090665521	previous	2%
future_whenallsucceed_100k_immediately_succeeded_off_loop	0.082102631	0.080239045	previous	2%
future_whenallsucceed_100k_immediately_succeeded_on_loop	0.08210656	0.079772289	previous	2%
future_whenallsucceed_10k_deferred_off_loop	0.023270075	0.023424646	current	0%
future_whenallsucceed_10k_deferred_on_loop	0.014501764	0.014184064	previous	2%
future_whenallcomplete_100k_immediately_succeeded_off_loop	0.03932471	0.040004257	current	-1%
future_whenallcomplete_100k_immediately_succeeded_on_loop	0.039345693	0.039849233	current	-1%
future_whenallcomplete_10k_deferred_off_loop	0.015512314	0.015505636	previous	0%
future_whenallcomplete_100k_deferred_on_loop	0.079030741	0.078745323	previous	0%
future_reduce_10k_futures	0.016651313	0.017077591	current	-2%
future_reduce_into_10k_futures	0.014419065	0.014645906	current	-1%
channel_pipeline_1m_events	0.097174262	0.097163114	previous	0%
websocket_encode_50b_space_at_front_100k_frames_cow	0.049522627	0.050747346	current	-2%
websocket_encode_50b_space_at_front_1m_frames_cow_masking	0.673983686	0.670778699	previous	0%
websocket_encode_1kb_space_at_front_1m_frames_cow	0.521021385	0.535278079	current	-2%
websocket_encode_50b_no_space_at_front_100k_frames_cow	0.049871428	0.050421533	current	-1%
websocket_encode_1kb_no_space_at_front_100k_frames_cow	0.052049898	0.053469907	current	-2%
websocket_encode_50b_space_at_front_100k_frames	0.075648943	0.072816206	previous	3%
websocket_encode_50b_space_at_front_10k_frames_masking	0.009278494	0.008729269	previous	6%
websocket_encode_1kb_space_at_front_10k_frames	0.012574961	0.012332604	previous	1%
websocket_encode_50b_no_space_at_front_100k_frames	0.075524543	0.072840072	previous	3%
websocket_encode_1kb_no_space_at_front_10k_frames	0.011946892	0.011691201	previous	2%
websocket_decode_125b_10k_frames	0.012627712	0.012615627	previous	0%
websocket_decode_125b_with_a_masking_key_10k_frames	0.013048638	0.01303065	previous	0%
websocket_decode_64kb_10k_frames	0.01295203	0.012978853	current	0%
websocket_decode_64kb_with_a_masking_key_10k_frames	0.013356652	0.013447025	current	0%
websocket_decode_64kb_+1_10k_frames	0.0129766	0.012896199	previous	0%
websocket_decode_64kb_+1_with_a_masking_key_10k_frames	0.013347262	0.01332532	previous	0%
circular_buffer_into_byte_buffer_1kb	0.033005886	0.033023522	current	0%
circular_buffer_into_byte_buffer_1mb	0.064681011	0.064696813	current	0%
byte_buffer_view_iterator_1mb	0.017561255	0.017566119	current	0%
byte_buffer_view_contains_12mb	0.052877447	0.052888283	current	0%
byte_to_message_decoder_decode_many_small	0.041698072	0.041169435	previous	1%
generate_10k_random_request_keys	0.090287404	0.090332135	current	0%
bytebuffer_rw_10_uint32s	0.041089399	0.04118375	current	0%
bytebuffer_multi_rw_10_uint32s	0.073854486	0.074681462	current	-1%
lock_1_thread_10M_ops	0.151240614	0.151585775	current	0%
lock_2_threads_10M_ops	0.834906333	0.813362241	previous	2%
lock_4_threads_10M_ops	0.891242095	0.857393789	previous	3%
lock_8_threads_10M_ops	0.975776571	0.872406368	previous	11%
schedule_100k_tasks	0.061631529	0.062143644	current	0%
schedule_and_run_100k_tasks	0.237549448	0.23035341	previous	3%
execute_100k_tasks	0.100502367	0.100370815	previous	0%
bytebufferview_copy_to_array_100k_times_1kb	0.011501877	0.011305817	previous	1%
circularbuffer_copy_to_array_10k_times_1kb	0.019785245	0.019783821	previous	0%
deadline_now_1M_times	0.024648632	0.024571329	previous	0%
asyncwriter_single_writes_1M_times	0.598652287	0.581300059	previous	2%
asyncsequenceproducer_consume_1M_times	0.826901793	0.83621323	current	-1%
udp_10k_writes	0.37734095	0.378068495	current	0%
udp_10k_vector_writes	0.204988632	0.20498118	previous	0%
udp_10k_vector_reads	0.386314055	0.387102143	current	0%
udp_10k_vector_reads_and_writes	0.108990303	0.107822741	previous	1%
tcp_100k_messages_throughput	0.902313439	0.767963419	previous	17%

significant differences found

[weissi]

@Lukasa / @jshier ugh, this is rough

no-net_http1_1k_reqs_1_conn | 0.013450951 | 0.011678896 | previous | 15%

15% slower with this change :|. And that includes client (unchanged) & servers (changed) meaning that servers will likely be affected even more gravely.

[Lukasa]

The change as written does runtime type checks. My review feedback above would replace it with a jump based on an enum value, which should perform a lot better.

[weissi]

The change as written does runtime type checks. My review feedback above would replace it with a jump based on an enum value, which should perform a lot better.

Missed that, sorry. That's great!

[jshier]

I've pushed the kind check up, feel free to test that, but I think there will still be an impact due to the data cast. I'll try to finish up the local tests soon.

[weissi]

@swift-server-bot test perf please

[swift-server-bot]

!!! Couldn't read commit file !!!

[weissi]

@swift-server-bot test perf please

[swift-server-bot]

!!! Couldn't read commit file !!!

[jshier]

I've added two tests, one for successful upgrade and one for failed upgrade.

@Lukasa What does a pipelined request pair look like? Do I just shove both through in one buffer?

[Lukasa]

What does a pipelined request pair look like? Do I just shove both through in one buffer?

Yeah, that's the easiest way to do it.

[jshier]

@Lukasa I can write a pipelined test for the current behavior just fine, as parsing is stopped as soon as the first request is parsed, but I'm not clear what behavior I should be seeing in the unsuccessful upgrade case. As far as I can tell, the data just disappears, and the channel is empty. I'm guessing that with parsing stopped, the additional data is just lost, and the unsuccessful upgrade turns parsing back on for a decoder that doesn't receive any additional data. To support the pipeline case I think I'd need to buffer any additional requests until we can tell whether the upgrade was successful, or some other change in behavior.

[Lukasa]

I don't think the data is lost, I think you need a way to re-spin the decoder. To test, can you try amending your current test case for the pipeline to send an extra empty ByteBuffer after you send the response that should re-enable parsing? My guess is that this will appear to "work", in which case my analysis is right.

[jshier]

@Lukasa You're correct, sending an empty buffer lets the test pass. Instead of just setting stopParsing = false, I'll also need to restart the parsing process.

[jshier]

@Lukasa I've investigated how to restart parsing in HTTPDecoder and there doesn't seem to be any way to do so simply. channel.writeInbound triggers a codepath the decoder can't reach, and since the decoder nils out the context when reading finishes, there's no way to start reading again internally. We could make the niling behavior dependent on the outgoing response, but I don't know if that's appropriate here. Any ideas what a proper fix might be here?