[GitHub] [qpid-dispatch] grs commented on a change in pull request #1229: DISPATCH-2142: buffering changes for tcp adaptor

[GitBox <gi...@apache.org>]

grs commented on a change in pull request #1229:
URL: https://github.com/apache/qpid-dispatch/pull/1229#discussion_r639747725



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -216,59 +226,39 @@ void qdr_tcp_q2_unblocked_handler(const qd_alloc_safe_ptr_t context)
     sys_mutex_unlock(tc->activation_lock);
 }
 
-
 // Extract buffers and their bytes from raw connection.
-// * Proton decides how many buffers are to be taken.
-// * Buffers with no data are freed.
-// * Buffers with data are appended to caller's buffers list.
 // * Add received byte count to connection stats
 // * Return the count of bytes in the buffers list
 static int handle_incoming_raw_read(qdr_tcp_connection_t *conn, qd_buffer_list_t *buffers)
 {
-    pn_raw_buffer_t raw_buffers[READ_BUFFERS];
-
-    size_t n;
-    int count = 0;
-    int free_count = 0;
-    const bool was_open = conn->bytes_unacked < TCP_MAX_CAPACITY;
-
-    while ((conn->raw_closed_write || count + conn->bytes_unacked < TCP_MAX_CAPACITY)
-           && (n = pn_raw_connection_take_read_buffers(conn->pn_raw_conn, raw_buffers, READ_BUFFERS)) ) {
-
-        for (size_t i = 0; i < n && raw_buffers[i].bytes; ++i) {
-            qd_buffer_t *buf = (qd_buffer_t*) raw_buffers[i].context;
-            qd_buffer_insert(buf, raw_buffers[i].size);
-            count += raw_buffers[i].size;
-
-            assert(raw_buffers[i].size == qd_buffer_size(buf));
-            if (raw_buffers[i].size > 0) {
-                DEQ_INSERT_TAIL(*buffers, buf);
-            } else {
-                qd_buffer_free(buf);
-                free_count++;
-            }
-        }
+    pn_raw_buffer_t raw_buffer;
+    if ( conn->bytes_unacked >= TCP_MAX_CAPACITY || !pn_raw_connection_take_read_buffers(conn->pn_raw_conn, &raw_buffer, 1)) {
+        return 0;
     }
+    int result = raw_buffer.size;
+    qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG,
+        "[C%"PRIu64"] pn_raw_connection_take_read_buffers() took buffer with %zu bytes",
+        conn->conn_id, result);
 
-    if (count > 0) {
+    if (buffers) {
+        qd_buffer_list_append(buffers, (uint8_t*) (raw_buffer.bytes + raw_buffer.offset), raw_buffer.size);
+    }
+    //reset buffer for further reads
+    conn->read_buffer.size = 0;
+    conn->read_buffer.offset = 0;
+    conn->read_pending = false;
+    if (result > 0) {
         // account for any incoming bytes just read
         conn->last_in_time = tcp_adaptor->core->uptime_ticks;
-        conn->bytes_in      += count;
-        conn->bytes_unacked += count;
+        conn->bytes_in      += result;
+        conn->bytes_unacked += result;

Review comment:
       I think if bytes_in is incremented, bytes_unacked should be also (even if there is never any possibility of them being acked), just for consistency.
   
   Incrementing bytes_in makes sense in one way, as it indicates the amount of data available to read. However I don't feel strongly about that as when the connection is closed (which it will be if it is being flushed), then at present the record of it is deleted.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org