Fix possible frame hole after preemtive repeat.

CMakeLists.txt

@@ -8,7 +8,7 @@ else()
 endif()
 
 project(picoquic
-        VERSION 1.1.11.0
+        VERSION 1.1.11.1
         DESCRIPTION "picoquic library"
         LANGUAGES C CXX)
 
@@ -131,6 +131,7 @@ set(PICOQUIC_LOGLIB_HEADERS loglib/autoqlog.h)
 
 set(PICOQUIC_TEST_LIBRARY_FILES
     picoquictest/ack_of_ack_test.c
+    picoquictest/app_limited.c
     picoquictest/bytestream_test.c
     picoquictest/cert_verify_test.c
     picoquictest/cleartext_aead_test.c

UnitTest1/unittest1.cpp

@@ -1866,6 +1866,30 @@ namespace UnitTest1
             Assert::AreEqual(ret, 0);
         }
 
+        TEST_METHOD(app_limited_bbr) {
+            int ret = app_limited_bbr_test();
+
+            Assert::AreEqual(ret, 0);
+        }
+
+        TEST_METHOD(app_limited_cubic) {
+            int ret = app_limited_cubic_test();
+
+            Assert::AreEqual(ret, 0);
+        }
+
+        TEST_METHOD(app_limited_reno) {
+            int ret = app_limited_reno_test();
+
+            Assert::AreEqual(ret, 0);
+        }
+
+        TEST_METHOD(app_limited_rpr) {
+            int ret = app_limited_rpr_test();
+
+            Assert::AreEqual(ret, 0);
+        }
+
         TEST_METHOD(cwin_max) {
             int ret = cwin_max_test();
 

picoquic/cubic.c

@@ -154,7 +154,6 @@ static void picoquic_cubic_enter_recovery(picoquic_cnx_t * cnx,
         path_x->cwin = PICOQUIC_CWIN_MINIMUM;
     }
     else {
-
         if (notification == picoquic_congestion_notification_timeout) {
             path_x->cwin = PICOQUIC_CWIN_MINIMUM;
             cubic_state->previous_start_of_epoch = cubic_state->start_of_epoch;

picoquic/frames.c

@@ -2783,7 +2783,7 @@ int picoquic_process_ack_of_ack_mp_frame(
 
 int picoquic_check_frame_needs_repeat(picoquic_cnx_t* cnx, const uint8_t* bytes,
     size_t bytes_max, picoquic_packet_type_enum p_type, 
-    int* no_need_to_repeat, int* do_not_detect_spurious, int is_preemptive)
+    int* no_need_to_repeat, int* do_not_detect_spurious, int *is_preemptive_needed)
 {
     int ret = 0;
     int fin;
@@ -2811,13 +2811,14 @@ int picoquic_check_frame_needs_repeat(picoquic_cnx_t* cnx, const uint8_t* bytes,
                 if (stream->reset_sent) {
                     *no_need_to_repeat = 1;
                 }
-                else if (is_preemptive && !stream->fin_sent) {
-                    *no_need_to_repeat = 1;
-                }
                 else {
                     /* Check whether the ack was already received */
                     *no_need_to_repeat = picoquic_check_sack_list(&stream->sack_list, offset, offset + data_length - ((fin) ? 0 : 1));
                 }
+
+                if (is_preemptive_needed != NULL && stream->fin_sent) {
+                    *is_preemptive_needed |= 1;
+                }
             }
         }
     }
@@ -2992,16 +2993,15 @@ int picoquic_check_frame_needs_repeat(picoquic_cnx_t* cnx, const uint8_t* bytes,
                     break;
                 case picoquic_frame_type_path_abandon:
                     /* TODO: check whether there is still a need to abandon the path */
-                    *no_need_to_repeat = is_preemptive;
+                    *no_need_to_repeat = 0;
                     break;
                 case picoquic_frame_type_path_status:
                     /* TODO: check whether there is not a status sent with a highest number
                      */
-                    *no_need_to_repeat = is_preemptive;
+                    *no_need_to_repeat = 0;
                     break;
                 default:
-                    /* If preemptive repeat, only repeat if the frame is explicitly required. */
-                    *no_need_to_repeat = is_preemptive;
+                    *no_need_to_repeat = 0;
                     break;
                 }
             }

picoquic/picoquic.h

@@ -40,7 +40,7 @@
 extern "C" {
 #endif
 
-#define PICOQUIC_VERSION "1.1.11.0"
+#define PICOQUIC_VERSION "1.1.11.1"
 #define PICOQUIC_ERROR_CLASS 0x400
 #define PICOQUIC_ERROR_DUPLICATE (PICOQUIC_ERROR_CLASS + 1)
 #define PICOQUIC_ERROR_AEAD_CHECK (PICOQUIC_ERROR_CLASS + 3)

picoquic/picoquic_internal.h

@@ -1781,7 +1781,7 @@ void picoquic_update_max_stream_ID_local(picoquic_cnx_t* cnx, picoquic_stream_he
  * May have to split a retransmitted stream frame if it does not fit in the new packet size */
 int picoquic_check_frame_needs_repeat(picoquic_cnx_t* cnx, const uint8_t* bytes,
     size_t bytes_max, picoquic_packet_type_enum p_type,
-    int* no_need_to_repeat, int* do_not_detect_spurious, int is_preemptive);
+    int* no_need_to_repeat, int* do_not_detect_spurious, int *is_preemptive_needed);
 uint8_t* picoquic_format_available_stream_frames(picoquic_cnx_t* cnx, picoquic_path_t * path_x,
     uint8_t* bytes_next, uint8_t* bytes_max,
     int* more_data, int* is_pure_ack, int* stream_tried_and_failed, int* ret);

picoquic/sender.c

@@ -1126,6 +1126,11 @@ picoquic_packet_t* picoquic_dequeue_retransmit_packet(picoquic_cnx_t* cnx,
     /* Remove from per path list */
     picoquic_dequeue_packet_from_path(p);
 
+    /* Replace head of preemptive repeat list if it was this packet. */
+    if (pkt_ctx->preemptive_repeat_ptr == p) {
+        pkt_ctx->preemptive_repeat_ptr = p->packet_next;
+    }
+
     if (should_free || p->is_ack_trap) {
         if (add_to_data_repeat_queue) {
             picoquic_queue_data_repeat_packet(cnx, p);
@@ -1473,7 +1478,7 @@ int picoquic_copy_before_retransmit(picoquic_packet_t * old_p,
              * case of spurious retransmissions */
             if (ret == 0 && frame_is_pure_ack == 0) {
                 ret = picoquic_check_frame_needs_repeat(cnx, &old_p->bytes[byte_index],
-                    frame_length, old_p->ptype, &frame_is_pure_ack, do_not_detect_spurious, 0);
+                    frame_length, old_p->ptype, &frame_is_pure_ack, do_not_detect_spurious, NULL);
             }
 
             /* Keep track of datagram frames that are possibly lost */
@@ -1966,7 +1971,17 @@ int picoquic_is_cnx_backlog_empty(picoquic_cnx_t* cnx)
     return backlog_empty;
 }
 
-/* Management of preemptive repeats
+/* Management of preemptive repeats.
+ * This function only perform preemptive repeat for packets that contain
+ * at least on frame that triggers premptive repeat, such as a stream
+ * belonging to a stream for which FIN was sent. If a packet is
+ * selected for preemptive repeat, then the function attempts to repeat
+ * all frames that are not "pure ack". If all such frames are repeated,
+ * the old packet can be marked as "was_preemptively_repeated", so that
+ * it will not be repeated if loss is detected. But if not all frames
+ * could be repeated, e.g., because of packet size, then the old packet
+ * must not be marked as preemptively repeated, because otherwise these
+ * non-repeated frames would be lost forever.
  */
 static int picoquic_preemptive_retransmit_packet(picoquic_packet_t* old_p,
     picoquic_cnx_t* cnx,
@@ -1983,6 +1998,8 @@ static int picoquic_preemptive_retransmit_packet(picoquic_packet_t* old_p,
     size_t write_index = 0;
     int is_repeated = 1;
     int do_not_detect_spurious = 0;
+    int is_preemptive_needed = 0;
+    size_t initial_length = *length;
     *has_data = 0;
 
     if (!old_p->is_mtu_probe &&
@@ -1999,7 +2016,7 @@ static int picoquic_preemptive_retransmit_packet(picoquic_packet_t* old_p,
              * case of spurious retransmissions */
             if (ret == 0 && frame_is_pure_ack == 0) {
                 ret = picoquic_check_frame_needs_repeat(cnx, &old_p->bytes[byte_index],
-                    frame_length, old_p->ptype, &frame_is_pure_ack, &do_not_detect_spurious, 1);
+                    frame_length, old_p->ptype, &frame_is_pure_ack, &do_not_detect_spurious, &is_preemptive_needed);
             }
 
             /* Prepare retransmission if needed */
@@ -2029,8 +2046,15 @@ static int picoquic_preemptive_retransmit_packet(picoquic_packet_t* old_p,
         }
     }
 
-    if (*has_data && is_repeated) {
-        old_p->was_preemptively_repeated = 1;
+    if (*has_data) {
+        if (!is_preemptive_needed) {
+            /* If the packet does not contain any frame requiring preemptive repeat, do not repeat it. */
+            *length = initial_length;
+            *has_data = 0;
+            is_repeated = 0;
+        } else if (is_repeated) {
+            old_p->was_preemptively_repeated = 1;
+        }
     }
 
     return ret;

picoquic_t/picoquic_t.c

@@ -330,6 +330,10 @@ static const picoquic_test_def_t test_table[] = {
     { "quality_update", quality_update_test },
     { "direct_receive", direct_receive_test },
     { "app_limit_cc", app_limit_cc_test },
+    { "app_limited_bbr", app_limited_bbr_test },
+    { "app_limited_cubic", app_limited_cubic_test },
+    { "app_limited_reno", app_limited_reno_test },
+    { "app_limited_rpr", app_limited_rpr_test },
     { "cwin_max", cwin_max_test },
     { "initial_race", initial_race_test },
     { "chacha20", chacha20_test },