From 04468d215dc184ae64e7aa1d0e940da50c724e2f Mon Sep 17 00:00:00 2001 From: Dmitri Tikhonov Date: Fri, 18 May 2018 15:29:07 -0400 Subject: [PATCH] Latest changes: - [API] Expose useful lsquic_ver2str[] in lsquic.h - [BUGFIX] Do not produce packet sequence gaps due to STREAM frame elision --- CHANGELOG | 2 + include/lsquic.h | 3 + src/liblsquic/lsquic_send_ctl.c | 46 +++------- src/liblsquic/lsquic_stream.c | 9 +- src/liblsquic/lsquic_stream.h | 5 ++ test/unittests/test_stream.c | 144 ++++++++++++++++++++++++++++++++ 6 files changed, 170 insertions(+), 39 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index f909b63..04ba03f 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,5 +1,7 @@ 2018-05-18 + - [API] Expose useful lsquic_ver2str[] in lsquic.h + - [BUGFIX] Do not produce packet sequence gaps due to STREAM frame elision - Improve checks of number of incoming streams limit and associated error reporting. - [BUGFIX] Command-line option `-6` now works correctly. diff --git a/include/lsquic.h b/include/lsquic.h index e0f5387..cf74361 100644 --- a/include/lsquic.h +++ b/include/lsquic.h @@ -890,6 +890,9 @@ enum LSQUIC_CONN_STATUS enum LSQUIC_CONN_STATUS lsquic_conn_status (lsquic_conn_t *, char *errbuf, size_t bufsz); +extern const char *const +lsquic_ver2str[N_LSQVER]; + #ifdef __cplusplus } #endif diff --git a/src/liblsquic/lsquic_send_ctl.c b/src/liblsquic/lsquic_send_ctl.c index d55397f..dabdc38 100644 --- a/src/liblsquic/lsquic_send_ctl.c +++ b/src/liblsquic/lsquic_send_ctl.c @@ -1322,28 +1322,6 @@ lsquic_send_ctl_set_tcid0 (lsquic_send_ctl_t *ctl, int tcid0) } -/* Need to assign new packet numbers to all packets following the first - * dropped packet to eliminate packet number gap. - */ -static void -send_ctl_repackno_sched_tail (struct lsquic_send_ctl *ctl, - struct lsquic_packet_out *pre_dropped) -{ - struct lsquic_packet_out *packet_out; - - assert(pre_dropped); - - ctl->sc_cur_packno = lsquic_senhist_largest(&ctl->sc_senhist); - for (packet_out = TAILQ_NEXT(pre_dropped, po_next); packet_out; - packet_out = TAILQ_NEXT(packet_out, po_next)) - { - packet_out->po_flags |= PO_REPACKNO; - if (packet_out->po_flags & PO_ENCRYPTED) - send_ctl_release_enc_data(ctl, packet_out); - } -} - - /* The controller elides this STREAM frames of stream `stream_id' from * scheduled and buffered packets. If a packet becomes empty as a result, * it is dropped. @@ -1356,10 +1334,10 @@ void lsquic_send_ctl_elide_stream_frames (lsquic_send_ctl_t *ctl, uint32_t stream_id) { struct lsquic_packet_out *packet_out, *next; - struct lsquic_packet_out *pre_dropped; unsigned n, adj; + int dropped; - pre_dropped = NULL; + dropped = 0; #ifdef WIN32 next = NULL; #endif @@ -1376,19 +1354,17 @@ lsquic_send_ctl_elide_stream_frames (lsquic_send_ctl_t *ctl, uint32_t stream_id) ctl->sc_bytes_scheduled -= adj; if (0 == packet_out->po_frame_types) { - if (!pre_dropped) - pre_dropped = TAILQ_PREV(packet_out, lsquic_packets_tailq, - po_next); LSQ_DEBUG("cancel packet %"PRIu64" after eliding frames for " "stream %"PRIu32, packet_out->po_packno, stream_id); send_ctl_sched_remove(ctl, packet_out); lsquic_packet_out_destroy(packet_out, ctl->sc_enpub); + ++dropped; } } } - if (pre_dropped) - send_ctl_repackno_sched_tail(ctl, pre_dropped); + if (dropped && ctl->sc_n_scheduled) + lsquic_send_ctl_reset_packnos(ctl); for (n = 0; n < sizeof(ctl->sc_buffered_packets) / sizeof(ctl->sc_buffered_packets[0]); ++n) @@ -1490,12 +1466,12 @@ int lsquic_send_ctl_squeeze_sched (lsquic_send_ctl_t *ctl) { struct lsquic_packet_out *packet_out, *next; - struct lsquic_packet_out *pre_dropped; + int dropped; #ifndef NDEBUG int pre_squeeze_logged = 0; #endif - pre_dropped = NULL; + dropped = 0; for (packet_out = TAILQ_FIRST(&ctl->sc_scheduled_packets); packet_out; packet_out = next) { @@ -1513,18 +1489,16 @@ lsquic_send_ctl_squeeze_sched (lsquic_send_ctl_t *ctl) LOG_PACKET_Q(&ctl->sc_scheduled_packets, "unacked packets before squeezing"); #endif - if (!pre_dropped) - pre_dropped = TAILQ_PREV(packet_out, lsquic_packets_tailq, - po_next); send_ctl_sched_remove(ctl, packet_out); LSQ_DEBUG("Dropping packet %"PRIu64" from scheduled queue", packet_out->po_packno); lsquic_packet_out_destroy(packet_out, ctl->sc_enpub); + ++dropped; } } - if (pre_dropped) - send_ctl_repackno_sched_tail(ctl, pre_dropped); + if (dropped && ctl->sc_n_scheduled) + lsquic_send_ctl_reset_packnos(ctl); #ifndef NDEBUG if (pre_squeeze_logged) diff --git a/src/liblsquic/lsquic_stream.c b/src/liblsquic/lsquic_stream.c index dd9feeb..a1496b3 100644 --- a/src/liblsquic/lsquic_stream.c +++ b/src/liblsquic/lsquic_stream.c @@ -1354,8 +1354,11 @@ lsquic_stream_flush (lsquic_stream_t *stream) /* The flush threshold is the maximum size of stream data that can be sent * in a full packet. */ -static size_t -flush_threshold (const lsquic_stream_t *stream) +#ifdef NDEBUG +static +#endif + size_t +lsquic_stream_flush_threshold (const struct lsquic_stream *stream) { enum packet_out_flags flags; enum lsquic_packno_bits bits; @@ -1716,7 +1719,7 @@ stream_write (lsquic_stream_t *stream, struct lsquic_reader *reader) { size_t thresh, len; - thresh = flush_threshold(stream); + thresh = lsquic_stream_flush_threshold(stream); len = reader->lsqr_size(reader->lsqr_ctx); if (stream->sm_n_buffered + len <= SM_BUF_SIZE && stream->sm_n_buffered + len < thresh) diff --git a/src/liblsquic/lsquic_stream.h b/src/liblsquic/lsquic_stream.h index c951df8..c010380 100644 --- a/src/liblsquic/lsquic_stream.h +++ b/src/liblsquic/lsquic_stream.h @@ -295,4 +295,9 @@ lsquic_stream_readable (const lsquic_stream_t *); size_t lsquic_stream_write_avail (const struct lsquic_stream *); +#ifndef NDEBUG +size_t +lsquic_stream_flush_threshold (const struct lsquic_stream *); +#endif + #endif diff --git a/test/unittests/test_stream.c b/test/unittests/test_stream.c index 6a4fdb6..111b251 100644 --- a/test/unittests/test_stream.c +++ b/test/unittests/test_stream.c @@ -915,6 +915,148 @@ test_loc_RST_rem_FIN (struct test_objs *tobjs) } +/* Test that when stream frame is elided and the packet is dropped, + * the send controller produces a gapless sequence. + * + * Case "middle": 3 packets with STREAM frames for streams A, B, and A. + * Stream B is reset. We should get a gapless sequence + * of packets 1, 2. + */ +static void +test_gapless_elision_middle (struct test_objs *tobjs) +{ + lsquic_stream_t *streamA, *streamB; + unsigned char buf[0x1000], buf_out[0x1000]; + size_t n, thresh, written_to_A = 0; + int s, fin; + lsquic_packet_out_t *packet_out; + + streamA = new_stream(tobjs, 345); + streamB = new_stream(tobjs, 347); + + init_buf(buf_out, sizeof(buf_out)); + thresh = lsquic_stream_flush_threshold(streamA); + n = lsquic_stream_write(streamA, buf_out, thresh); + assert(n == thresh); + assert(1 == lsquic_send_ctl_n_scheduled(&tobjs->send_ctl)); + written_to_A += n; + + thresh = lsquic_stream_flush_threshold(streamB); + n = lsquic_stream_write(streamB, buf_out, thresh); + assert(n == thresh); + assert(2 == lsquic_send_ctl_n_scheduled(&tobjs->send_ctl)); + + thresh = lsquic_stream_flush_threshold(streamA); + n = lsquic_stream_write(streamA, buf_out + written_to_A, thresh); + assert(n == thresh); + assert(3 == lsquic_send_ctl_n_scheduled(&tobjs->send_ctl)); + written_to_A += n; + + /* Verify contents of A: */ + n = read_from_scheduled_packets(&tobjs->send_ctl, streamA->id, buf, + sizeof(buf), 0, &fin, 0); + assert(n == written_to_A); + assert(0 == memcmp(buf, buf_out, written_to_A)); + + /* Now reset stream A: */ + s = lsquic_stream_rst_in(streamB, 0, 0); + assert(s == 0); + assert(2 == lsquic_send_ctl_n_scheduled(&tobjs->send_ctl)); + /* Verify A again: */ + n = read_from_scheduled_packets(&tobjs->send_ctl, streamA->id, buf, + sizeof(buf), 0, &fin, 0); + assert(n == written_to_A); + assert(0 == memcmp(buf, buf_out, written_to_A)); + + packet_out = lsquic_send_ctl_next_packet_to_send(&tobjs->send_ctl); + assert(packet_out->po_packno == 1); + lsquic_send_ctl_sent_packet(&tobjs->send_ctl, packet_out, 1); + + packet_out = lsquic_send_ctl_next_packet_to_send(&tobjs->send_ctl); + assert(packet_out->po_packno == 2); + lsquic_send_ctl_sent_packet(&tobjs->send_ctl, packet_out, 1); + + packet_out = lsquic_send_ctl_next_packet_to_send(&tobjs->send_ctl); + assert(!packet_out); + + /* Now we can call on_close: */ + lsquic_stream_destroy(streamA); + lsquic_stream_destroy(streamB); +} + + +/* Test that when stream frame is elided and the packet is dropped, + * the send controller produces a gapless sequence. + * + * Case "beginnig": 3 packets with STREAM frames for streams B, A, and A. + * Stream B is reset. We should get a gapless sequence + * of packets 1, 2. + */ +static void +test_gapless_elision_beginning (struct test_objs *tobjs) +{ + lsquic_stream_t *streamA, *streamB; + unsigned char buf[0x1000], buf_out[0x1000]; + size_t n, thresh, written_to_A = 0; + int s, fin; + lsquic_packet_out_t *packet_out; + + streamA = new_stream(tobjs, 345); + streamB = new_stream(tobjs, 347); + + init_buf(buf_out, sizeof(buf_out)); + + thresh = lsquic_stream_flush_threshold(streamB); + n = lsquic_stream_write(streamB, buf_out, thresh); + assert(n == thresh); + assert(1 == lsquic_send_ctl_n_scheduled(&tobjs->send_ctl)); + + thresh = lsquic_stream_flush_threshold(streamA); + n = lsquic_stream_write(streamA, buf_out, thresh); + assert(n == thresh); + assert(2 == lsquic_send_ctl_n_scheduled(&tobjs->send_ctl)); + written_to_A += n; + + thresh = lsquic_stream_flush_threshold(streamA); + n = lsquic_stream_write(streamA, buf_out + written_to_A, thresh); + assert(n == thresh); + assert(3 == lsquic_send_ctl_n_scheduled(&tobjs->send_ctl)); + written_to_A += n; + + /* Verify contents of A: */ + n = read_from_scheduled_packets(&tobjs->send_ctl, streamA->id, buf, + sizeof(buf), 0, &fin, 0); + assert(n == written_to_A); + assert(0 == memcmp(buf, buf_out, written_to_A)); + + /* Now reset stream A: */ + s = lsquic_stream_rst_in(streamB, 0, 0); + assert(s == 0); + assert(2 == lsquic_send_ctl_n_scheduled(&tobjs->send_ctl)); + /* Verify A again: */ + n = read_from_scheduled_packets(&tobjs->send_ctl, streamA->id, buf, + sizeof(buf), 0, &fin, 0); + assert(n == written_to_A); + assert(0 == memcmp(buf, buf_out, written_to_A)); + + packet_out = lsquic_send_ctl_next_packet_to_send(&tobjs->send_ctl); + assert(packet_out->po_packno == 1); + lsquic_send_ctl_sent_packet(&tobjs->send_ctl, packet_out, 1); + + packet_out = lsquic_send_ctl_next_packet_to_send(&tobjs->send_ctl); + assert(packet_out->po_packno == 2); + lsquic_send_ctl_sent_packet(&tobjs->send_ctl, packet_out, 1); + + packet_out = lsquic_send_ctl_next_packet_to_send(&tobjs->send_ctl); + assert(!packet_out); + + /* Now we can call on_close: */ + lsquic_stream_destroy(streamA); + lsquic_stream_destroy(streamB); +} + + + /* Write data to the stream, but do not flush: connection cap take a hit. * After stream is destroyed, connection cap should go back up. */ @@ -1045,6 +1187,8 @@ test_termination (void) test_loc_FIN_rem_RST, test_loc_data_rem_RST, test_loc_RST_rem_FIN, + test_gapless_elision_beginning, + test_gapless_elision_middle, }; for (i = 0; i < sizeof(test_funcs) / sizeof(test_funcs[0]); ++i)