You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2021/04/15 23:49:17 UTC

[GitHub] [qpid-dispatch] ChugR opened a new pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

ChugR opened a new pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129


   DISPATCH-2043: Various TCP Adaptor logging improvements
   
   * monitor and control sequence of half closed events
   * raw read caches data if read_close before stream message created
   * raw write defers EOS write_close until stream message created
   
   This version leaks buffers at the rate of about 15 per connection pair.
   The suspect is that proton never emits a PN_RAW_CONNECTION_READ after
   emitting CLOSED_READ and this code never gets to reclaim the buffers.
   That said, this code could have a bug in that area as well.


-- 
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

[GitHub] [qpid-dispatch] grs commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r615061964



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -782,6 +965,14 @@ static void qdr_tcp_open_server_side_connection(qdr_tcp_connection_t* tc)
 
     // This attach passes the ownership of the delivery from the core-side connection and link
     // to the adaptor-side outgoing connection and link.
+    uint64_t i_conn_id = 0;
+    uint64_t i_link_id = 0;
+    uint64_t i_dlv_id = 0;
+    if (!!tc->initial_delivery) {
+        i_conn_id = tc->initial_delivery->conn_id;

Review comment:
       Sorry, I still don't get it. The i_dlv_id is not referenced in the qdr_link_first_attach call.




-- 
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

[GitHub] [qpid-dispatch] grs commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r615073245



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -1257,13 +1489,17 @@ static void qdr_tcp_delivery_update(void *context, qdr_delivery_t *dlv, uint64_t
     void* link_context = qdr_link_get_context(qdr_delivery_link(dlv));
     if (link_context) {
         qdr_tcp_connection_t* tc = (qdr_tcp_connection_t*) link_context;
-        qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG, DLV_FMT" qdr_tcp_delivery_update: disp: %"PRIu64", settled: %s",
+        qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG,
+               DLV_FMT" qdr_tcp_delivery_update: disp: %"PRIu64", settled: %s",
                DLV_ARGS(dlv), disp, settled ? "true" : "false");
 
         //
         // If one of the streaming deliveries is ever settled, the connection must be torn down.

Review comment:
       That is exactly my point. The same is true for the other direction, a tcpListener CLOSED_WRITE should be propagated to the peer tcpConnector where it will call read_close(). The CLOSED_WRITE at the tcpConnector will be propagated to the tcpListener for it to call read_close(). Read and write streams are chained in both directions.
   
   I argue that the comment above is actually short circuiting this (as it predates the API to handle read_close()/write_close() separately). Though it may work for the ncat case, that short circuiting seems less clean than a proper symmetric chaining.




-- 
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

[GitHub] [qpid-dispatch] grs commented on pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
grs commented on pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#issuecomment-824779284


   > Problem: This branch hangs if there is no server
   
   I believe I have a fix for this in https://github.com/ChugR/qpid-dispatch/pull/81
   
   > On the connector side server connection there are two events:
   > 
   > * pn_proactor_raw_connect()
   > * DISCONNECTED event
   > 
   > The tcp connector side must go through enough initialization so that the listener-connector links are established. Then the connector side can tear down the links and the client connection will close.
   
   If the DISCONNECTED happens before CONNECTED, then the initial_delivery is released. The fix above ensures this is handled correctly and the connection is closed.
   


-- 
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

[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
ChugR commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r615490965



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -1257,13 +1489,17 @@ static void qdr_tcp_delivery_update(void *context, qdr_delivery_t *dlv, uint64_t
     void* link_context = qdr_link_get_context(qdr_delivery_link(dlv));
     if (link_context) {
         qdr_tcp_connection_t* tc = (qdr_tcp_connection_t*) link_context;
-        qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG, DLV_FMT" qdr_tcp_delivery_update: disp: %"PRIu64", settled: %s",
+        qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG,
+               DLV_FMT" qdr_tcp_delivery_update: disp: %"PRIu64", settled: %s",
                DLV_ARGS(dlv), disp, settled ? "true" : "false");
 
         //
         // If one of the streaming deliveries is ever settled, the connection must be torn down.

Review comment:
       Maybe a solution would be not to accept an incoming client raw connection until after all of the TCP adaptor mechanisms have been set up. This would stall the initial connection setup but it would simplify propagating closed read/write in both directions when they happen.
   




-- 
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

[GitHub] [qpid-dispatch] codecov-commenter commented on pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#issuecomment-822536755


   # [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#1129](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7253dfb) into [main](https://codecov.io/gh/apache/qpid-dispatch/commit/f67e135c0cfd579272d191b0e14342c0f4cd8cc6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f67e135) will **decrease** coverage by `0.24%`.
   > The diff coverage is `80.76%`.
   
   > :exclamation: Current head 7253dfb differs from pull request most recent head 0005f20. Consider uploading reports for the commit 0005f20 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/graphs/tree.svg?width=650&height=150&src=pr&token=rk2Cgd27pP&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##             main    #1129      +/-   ##
   ==========================================
   - Coverage   84.37%   84.12%   -0.25%     
   ==========================================
     Files         111      111              
     Lines       27536    27629      +93     
   ==========================================
   + Hits        23233    23243      +10     
   - Misses       4303     4386      +83     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [src/adaptors/tcp\_adaptor.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL2FkYXB0b3JzL3RjcF9hZGFwdG9yLmM=) | `67.74% <80.76%> (-1.40%)` | :arrow_down: |
   | [src/iterator.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL2l0ZXJhdG9yLmM=) | `89.29% <0.00%> (-3.88%)` | :arrow_down: |
   | [src/router\_node.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9ub2RlLmM=) | `92.85% <0.00%> (-0.74%)` | :arrow_down: |
   | [src/router\_core/connections.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL2Nvbm5lY3Rpb25zLmM=) | `89.21% <0.00%> (-0.71%)` | :arrow_down: |
   | [src/router\_core/router\_core.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL3JvdXRlcl9jb3JlLmM=) | `86.14% <0.00%> (-0.59%)` | :arrow_down: |
   | [src/router\_core/forwarder.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL2ZvcndhcmRlci5j) | `92.67% <0.00%> (-0.40%)` | :arrow_down: |
   | [src/router\_core/delivery.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL2RlbGl2ZXJ5LmM=) | `92.74% <0.00%> (-0.39%)` | :arrow_down: |
   | [src/adaptors/http1/http1\_server.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL2FkYXB0b3JzL2h0dHAxL2h0dHAxX3NlcnZlci5j) | `84.59% <0.00%> (-0.30%)` | :arrow_down: |
   | [src/router\_core/transfer.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL3RyYW5zZmVyLmM=) | `93.96% <0.00%> (-0.22%)` | :arrow_down: |
   | ... and [37 more](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f67e135...0005f20](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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

[GitHub] [qpid-dispatch] grs commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r618302219



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -1257,13 +1489,17 @@ static void qdr_tcp_delivery_update(void *context, qdr_delivery_t *dlv, uint64_t
     void* link_context = qdr_link_get_context(qdr_delivery_link(dlv));
     if (link_context) {
         qdr_tcp_connection_t* tc = (qdr_tcp_connection_t*) link_context;
-        qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG, DLV_FMT" qdr_tcp_delivery_update: disp: %"PRIu64", settled: %s",
+        qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG,
+               DLV_FMT" qdr_tcp_delivery_update: disp: %"PRIu64", settled: %s",
                DLV_ARGS(dlv), disp, settled ? "true" : "false");
 
         //
         // If one of the streaming deliveries is ever settled, the connection must be torn down.

Review comment:
       https://github.com/ChugR/qpid-dispatch/pull/81
   




-- 
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

[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
ChugR commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r615047802



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -116,6 +120,26 @@ static inline uint64_t qdr_tcp_conn_linkid(const qdr_tcp_connection_t *conn)
     return conn->instream ? conn->incoming_id : conn->outgoing_id;
 }
 
+static inline const char * qdr_link_direction_name(const qdr_link_t *link)
+{
+    assert(link);
+    return qdr_link_direction(link) == QD_OUTGOING ? "outgoing" : "incoming";
+}
+
+static inline const char * qdr_tcp_connection_role_name(const qdr_tcp_connection_t *tc)
+{
+    assert(tc);
+    return tc->ingress ? "client" : "server";

Review comment:
       Ambiguity abounds no matter how it is described. I figure that a client originates a connection to a listener and that a connector originates a connection to a server. Clients never accept connections and servers never originate connections. 
   Also, there was some bad code logic in these descriptions that caused the logs to say wrong things. An update is coming shortly.




-- 
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

[GitHub] [qpid-dispatch] asfgit closed pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129


   


-- 
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

[GitHub] [qpid-dispatch] grs commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r614998347



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -288,18 +338,87 @@ static int handle_incoming_impl(qdr_tcp_connection_t *conn, bool close_pending)
         qd_message_set_q2_unblocked_handler(msg, qdr_tcp_q2_unblocked_handler, conn_sp);
 
         conn->instream = qdr_link_deliver(conn->incoming, msg, 0, false, 0, 0, 0, 0);
-        qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG, "[C%"PRIu64"][L%"PRIu64"] Initiating message with %i bytes", conn->conn_id, conn->incoming_id, count);
+        qd_log(log, QD_LOG_DEBUG,
+               "[C%"PRIu64"][L%"PRIu64"][D%"PRIu64"] Initiating ingress stream message with %u bytes",
+               conn->conn_id, conn->incoming_id, conn->instream->delivery_id, length);
+        conn->incoming_started = true;
+
+        // Handle deferment of write side close.
+        sys_mutex_lock(conn->activation_lock);
+        if (conn->read_eos_seen && !conn->raw_closed_write) {
+            // to-raw-conn EOS was seen before the from-raw-conn instream delivery existed.

Review comment:
       Why do we need to wait for the instream delivery before calling write_close()?




-- 
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

[GitHub] [qpid-dispatch] ChugR commented on pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
ChugR commented on pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#issuecomment-824774480


   Problem: This branch hangs if there is no server
   There is no useful comparison between main branch, which does not hang, and this branch. Main branch closes the client connection when the connection is closed for reading whether there is a server or not.
   On the connector side server connection there are two events:
   
   - pn_proactor_raw_connect() 
   - DISCONNECTED event
   
   The tcp connector side must go through enough initialization so that the listener-connector links are established. Then the connector side can tear down the links and the client connection will close.


-- 
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

[GitHub] [qpid-dispatch] grs commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r615108651



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -288,18 +338,87 @@ static int handle_incoming_impl(qdr_tcp_connection_t *conn, bool close_pending)
         qd_message_set_q2_unblocked_handler(msg, qdr_tcp_q2_unblocked_handler, conn_sp);
 
         conn->instream = qdr_link_deliver(conn->incoming, msg, 0, false, 0, 0, 0, 0);
-        qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG, "[C%"PRIu64"][L%"PRIu64"] Initiating message with %i bytes", conn->conn_id, conn->incoming_id, count);
+        qd_log(log, QD_LOG_DEBUG,
+               "[C%"PRIu64"][L%"PRIu64"][D%"PRIu64"] Initiating ingress stream message with %u bytes",
+               conn->conn_id, conn->incoming_id, conn->instream->delivery_id, length);
+        conn->incoming_started = true;
+
+        // Handle deferment of write side close.
+        sys_mutex_lock(conn->activation_lock);
+        if (conn->read_eos_seen && !conn->raw_closed_write) {
+            // to-raw-conn EOS was seen before the from-raw-conn instream delivery existed.

Review comment:
       If C3110  is on the tcpConnector, then it does not need to wait for reply-to in order to create the reply message as that was in the message it already received and read to EOS. If it does not have credit it should not have granted read buffers and so would not get the READ events until it did have credit.




-- 
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

[GitHub] [qpid-dispatch] grs commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r615069069



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -57,11 +57,15 @@ struct qdr_tcp_connection_t {
     qdr_delivery_t       *outstream;
     bool                  ingress;
     bool                  flow_enabled;
+    bool                  incoming_started;
     bool                  egress_dispatcher;
     bool                  connector_closed;//only used if egress_dispatcher=true
     bool                  in_list;         // This connection is in the adaptor's connections list
-    bool                  raw_closed_read;
-    bool                  raw_closed_write;
+    bool                  raw_closed_read;   // proton event seen
+    bool                  raw_closed_write;  // proton event seen or write_close called
+    bool                  raw_read_shutdown; // stream closed
+    bool                  read_eos_seen;
+    qd_buffer_list_t      early_raw_read_bufs; // read from raw conn before ingress stream ready

Review comment:
       I don't believe we would need a wake as we would be creating the instream on the io thread anyway. All that is needed is to drain the data available and append it to the instream.
   
   We can't get a READ event if we don't grant any buffers anyway. Presumably the READ_CLOSED event must come after we have read all the data, else how would we know there was no more left. @astitcher ?




-- 
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

[GitHub] [qpid-dispatch] ChugR commented on pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
ChugR commented on pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#issuecomment-824063244


   Current code branch DISPATCH-1878-04 at commit 77765 has an issue when there is no server.
   * On main branch the command _echo abcd | nc localhost 9999_ exits without printing anything
   * With this PR the command hangs without printing anything
   This issue should be corrected.


-- 
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

[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
ChugR commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r615929632



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -630,64 +780,92 @@ static void handle_connection_event(pn_event_t *e, qd_server_t *qd_server, void
         }
     }
     case PN_RAW_CONNECTION_CLOSED_READ: {
-        qd_log(log, QD_LOG_DEBUG, "[C%"PRIu64"] PN_RAW_CONNECTION_CLOSED_READ", conn->conn_id);
-        conn->q2_blocked = false;
-        handle_incoming_impl(conn, true);
+        qd_log(log, QD_LOG_DEBUG, "[C%"PRIu64"][L%"PRIu64"] PN_RAW_CONNECTION_CLOSED_READ",
+               conn->conn_id, conn->incoming_id);
         sys_mutex_lock(conn->activation_lock);
+        conn->q2_blocked = false;
         conn->raw_closed_read = true;
         sys_mutex_unlock(conn->activation_lock);
-        pn_raw_connection_close(conn->pn_raw_conn);
+        handle_incoming(conn, "PNRC_CLOSED_READ");
         break;
     }
     case PN_RAW_CONNECTION_CLOSED_WRITE: {
-        qd_log(log, QD_LOG_DEBUG, "[C%"PRIu64"] PN_RAW_CONNECTION_CLOSED_WRITE", conn->conn_id);
+        qd_log(log, QD_LOG_DEBUG,
+               "[C%"PRIu64"] PN_RAW_CONNECTION_CLOSED_WRITE",
+               conn->conn_id);
         sys_mutex_lock(conn->activation_lock);
         conn->raw_closed_write = true;
         sys_mutex_unlock(conn->activation_lock);
-        pn_raw_connection_close(conn->pn_raw_conn);
+        if (conn->ingress) {

Review comment:
       Getting rid of the conditional AND the pn_connection_close() call seems to work correctly. Just remove the code block entirely.




-- 
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

[GitHub] [qpid-dispatch] codecov-commenter edited a comment on pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#issuecomment-822536755


   # [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#1129](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c77b180) into [main](https://codecov.io/gh/apache/qpid-dispatch/commit/f67e135c0cfd579272d191b0e14342c0f4cd8cc6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f67e135) will **decrease** coverage by `0.10%`.
   > The diff coverage is `82.68%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/graphs/tree.svg?width=650&height=150&src=pr&token=rk2Cgd27pP&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##             main    #1129      +/-   ##
   ==========================================
   - Coverage   84.37%   84.27%   -0.11%     
   ==========================================
     Files         111      111              
     Lines       27536    27634      +98     
   ==========================================
   + Hits        23233    23288      +55     
   - Misses       4303     4346      +43     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [src/adaptors/tcp\_adaptor.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL2FkYXB0b3JzL3RjcF9hZGFwdG9yLmM=) | `68.21% <82.68%> (-0.93%)` | :arrow_down: |
   | [src/router\_node.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9ub2RlLmM=) | `92.75% <0.00%> (-0.84%)` | :arrow_down: |
   | [src/router\_core/connections.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL2Nvbm5lY3Rpb25zLmM=) | `89.61% <0.00%> (-0.31%)` | :arrow_down: |
   | [src/adaptors/http1/http1\_server.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL2FkYXB0b3JzL2h0dHAxL2h0dHAxX3NlcnZlci5j) | `84.59% <0.00%> (-0.30%)` | :arrow_down: |
   | [src/adaptors/http1/http1\_codec.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL2FkYXB0b3JzL2h0dHAxL2h0dHAxX2NvZGVjLmM=) | `85.15% <0.00%> (-0.13%)` | :arrow_down: |
   | [src/message.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21lc3NhZ2UuYw==) | `86.59% <0.00%> (-0.08%)` | :arrow_down: |
   | [src/hash.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL2hhc2guYw==) | `79.34% <0.00%> (ø)` | |
   | [src/parse.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3BhcnNlLmM=) | `86.87% <0.00%> (ø)` | |
   | [src/policy.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3BvbGljeS5j) | `87.29% <0.00%> (ø)` | |
   | ... and [37 more](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f67e135...c77b180](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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

[GitHub] [qpid-dispatch] grs commented on pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
grs commented on pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#issuecomment-824062039


   Looking good! I posted a patch with a few simplifications: https://github.com/ChugR/qpid-dispatch/pull/76. That passes all local tests and has got through more than 500,000 ncat tests without issue (still running fine).


-- 
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

[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
ChugR commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r615043535



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -57,11 +57,15 @@ struct qdr_tcp_connection_t {
     qdr_delivery_t       *outstream;
     bool                  ingress;
     bool                  flow_enabled;
+    bool                  incoming_started;
     bool                  egress_dispatcher;
     bool                  connector_closed;//only used if egress_dispatcher=true
     bool                  in_list;         // This connection is in the adaptor's connections list
-    bool                  raw_closed_read;
-    bool                  raw_closed_write;
+    bool                  raw_closed_read;   // proton event seen
+    bool                  raw_closed_write;  // proton event seen or write_close called
+    bool                  raw_read_shutdown; // stream closed
+    bool                  read_eos_seen;
+    qd_buffer_list_t      early_raw_read_bufs; // read from raw conn before ingress stream ready

Review comment:
       This was on recommendation from proton developers: giving buffers to the read side is pretty harmless.
   
   Reading from raw connection before the ingress stream is ready is the first strategy of several that has worked. Over the wire the client has executed open-write-close before the TCP adaptor has even been notified that the connection has been accepted. Then follows a READ event which TCP can't handle since it has no stream or credit. Then follows a READ_CLOSED which still can't be handled since there is no stream or credit. Eventually the stream comes up and credit arrives but these events don't arrive in the proper context for reading the raw connection. One could schedule a wake but complicated state is required to signal that on a random wake it's time to read from a closed connection. 




-- 
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

[GitHub] [qpid-dispatch] grs commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r615060537



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -116,6 +120,26 @@ static inline uint64_t qdr_tcp_conn_linkid(const qdr_tcp_connection_t *conn)
     return conn->instream ? conn->incoming_id : conn->outgoing_id;
 }
 
+static inline const char * qdr_link_direction_name(const qdr_link_t *link)
+{
+    assert(link);
+    return qdr_link_direction(link) == QD_OUTGOING ? "outgoing" : "incoming";
+}
+
+static inline const char * qdr_tcp_connection_role_name(const qdr_tcp_connection_t *tc)
+{
+    assert(tc);
+    return tc->ingress ? "client" : "server";

Review comment:
       > Clients never accept connections and servers never originate connections.
   
   But that is not what the line above implies, is it? If ingress=true it is a connection managed by a TcpListener, i,e, it was an accepted connection.




-- 
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

[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
ChugR commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r615055634



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -782,6 +965,14 @@ static void qdr_tcp_open_server_side_connection(qdr_tcp_connection_t* tc)
 
     // This attach passes the ownership of the delivery from the core-side connection and link
     // to the adaptor-side outgoing connection and link.
+    uint64_t i_conn_id = 0;
+    uint64_t i_link_id = 0;
+    uint64_t i_dlv_id = 0;
+    if (!!tc->initial_delivery) {
+        i_conn_id = tc->initial_delivery->conn_id;

Review comment:
       qdr_link_first_attach conditionally "adjusts" the delivery's identity. This code is conditionally memorizing what the initial delivery had before the adjustment.
   The log statement giving identical [Cx]{Lx][Dx] values before and after was pretty lame.




-- 
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

[GitHub] [qpid-dispatch] grs commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r615089829



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -288,18 +338,87 @@ static int handle_incoming_impl(qdr_tcp_connection_t *conn, bool close_pending)
         qd_message_set_q2_unblocked_handler(msg, qdr_tcp_q2_unblocked_handler, conn_sp);
 
         conn->instream = qdr_link_deliver(conn->incoming, msg, 0, false, 0, 0, 0, 0);
-        qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG, "[C%"PRIu64"][L%"PRIu64"] Initiating message with %i bytes", conn->conn_id, conn->incoming_id, count);
+        qd_log(log, QD_LOG_DEBUG,
+               "[C%"PRIu64"][L%"PRIu64"][D%"PRIu64"] Initiating ingress stream message with %u bytes",
+               conn->conn_id, conn->incoming_id, conn->instream->delivery_id, length);
+        conn->incoming_started = true;
+
+        // Handle deferment of write side close.
+        sys_mutex_lock(conn->activation_lock);
+        if (conn->read_eos_seen && !conn->raw_closed_write) {
+            // to-raw-conn EOS was seen before the from-raw-conn instream delivery existed.

Review comment:
       That sounds like the problem is that we handle CLOSED_WRITE by closing the whole connection, rather than simply relaying what the two endpoints themselves do.




-- 
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

[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
ChugR commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r615051274



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -288,18 +338,87 @@ static int handle_incoming_impl(qdr_tcp_connection_t *conn, bool close_pending)
         qd_message_set_q2_unblocked_handler(msg, qdr_tcp_q2_unblocked_handler, conn_sp);
 
         conn->instream = qdr_link_deliver(conn->incoming, msg, 0, false, 0, 0, 0, 0);
-        qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG, "[C%"PRIu64"][L%"PRIu64"] Initiating message with %i bytes", conn->conn_id, conn->incoming_id, count);
+        qd_log(log, QD_LOG_DEBUG,
+               "[C%"PRIu64"][L%"PRIu64"][D%"PRIu64"] Initiating ingress stream message with %u bytes",
+               conn->conn_id, conn->incoming_id, conn->instream->delivery_id, length);
+        conn->incoming_started = true;
+
+        // Handle deferment of write side close.
+        sys_mutex_lock(conn->activation_lock);
+        if (conn->read_eos_seen && !conn->raw_closed_write) {
+            // to-raw-conn EOS was seen before the from-raw-conn instream delivery existed.

Review comment:
       Because if the instream creation is too slow then close the raw connection becomes WRITE_CLOSED, then the server sends the response and closes it's connection, the raw connection becomes READ_CLOSED, and DISCONNECTED and everything is discarded.
   All this happens while waiting for the instream to come up and get credit.
   By deferring the write-close until the instream comes up then any server response has a place to go before the raw connection gets closed.
   On a single router test this condition would show up after maybe 500 or 3000 successful tries. It was hard to track down, easy to fix.




-- 
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

[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
ChugR commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r615103944



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -288,18 +338,87 @@ static int handle_incoming_impl(qdr_tcp_connection_t *conn, bool close_pending)
         qd_message_set_q2_unblocked_handler(msg, qdr_tcp_q2_unblocked_handler, conn_sp);
 
         conn->instream = qdr_link_deliver(conn->incoming, msg, 0, false, 0, 0, 0, 0);
-        qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG, "[C%"PRIu64"][L%"PRIu64"] Initiating message with %i bytes", conn->conn_id, conn->incoming_id, count);
+        qd_log(log, QD_LOG_DEBUG,
+               "[C%"PRIu64"][L%"PRIu64"][D%"PRIu64"] Initiating ingress stream message with %u bytes",
+               conn->conn_id, conn->incoming_id, conn->instream->delivery_id, length);
+        conn->incoming_started = true;
+
+        // Handle deferment of write side close.
+        sys_mutex_lock(conn->activation_lock);
+        if (conn->read_eos_seen && !conn->raw_closed_write) {
+            // to-raw-conn EOS was seen before the from-raw-conn instream delivery existed.

Review comment:
       From a trace the other day.
   
   ```
   TCP Adaptor half-closed hang after 1500 connections
   
    on the server side connection [C3110] is tcpConnector connection:
    * connection opens
    * client data 5 bytes is written
    * client connection stream to server is closed (EOS)
    * server connection does WRITE_CLOSE (same as original client does open-write-close to tcpListener)
    * server replies with 21 bytes
    * server caches the 21 bytes since "Waiting for credit before initiating ingress stream message"
      there's no message to put the bytes into yet
    * server application closes after writing the 21 byte reply
    
    * with server connection WRITE_CLOSED and READ_CLOSED the CONNECTION_DISCONNECTED comes along
    * handle_disconnected tears everything down and the cached bytes never get written into stream message
    
    * There's a bug since the server connection should stay up long enough to write the cached bytes to the stream message
    
   TCP_ADAPTOR (debug) [C3110][L6219] (server outgoing) Created link to amqp:/_topo/0/D-1878/temp.RHkTIA2kerAComs
   TCP_ADAPTOR (trace) [C3110] handle_incoming qdr_tcp_deliver for server connection. read_closed:F, flow_enabled:F
   TCP_ADAPTOR (debug) [C3110][L6218] Waiting for credit before initiating ingress stream message
   TCP_ADAPTOR (debug) [C3110] pn_raw_connection_write_buffers wrote 5 bytes
   TCP_ADAPTOR (info) [C3110] EOS
       here the adaptor calls write_close() to server connection when EOS is seen
   TCP_ADAPTOR (debug) [C3110] handle_outgoing calling pn_raw_connection_write_close(). rcv_complete:T, send_complete:T
   TCP_ADAPTOR (debug) [C3110][L6218] qdr_tcp_offer: NOOP
   TCP_ADAPTOR (debug) [C3110][L6218] qdr_tcp_get_credit: NOOP
   TCP_ADAPTOR (debug) [C3110][L6218] (server outgoing) qdr_tcp_second_attach
   TCP_ADAPTOR (debug) [C3110][L6218] qdr_tcp_get_credit: NOOP
   TCP_ADAPTOR (debug) [C3110] PN_RAW_CONNECTION_CLOSED_WRITE
       adaptor connector to server is now write closed
   TCP_ADAPTOR (debug) [C3110] PN_RAW_CONNECTION_WRITTEN pn_raw_connection_take_written_buffers wrote 5 bytes. Total written 5 bytes
   TCP_ADAPTOR (debug) [C3110] pn_raw_connection_take_read_buffers() took 1, freed 0
   TCP_ADAPTOR (debug) [C3110] PN_RAW_CONNECTION_READ Read 21 bytes. Total read 21 bytes
   TCP_ADAPTOR (debug) [C3110] PN_RAW_CONNECTION_READ Read 0 bytes. Total read 21 bytes
       server echos the 5 bytes with a prefix to make it 21 bytes. Then server closes connection.
   TCP_ADAPTOR (debug) [C3110][L6219] PN_RAW_CONNECTION_CLOSED_READ
   TCP_ADAPTOR (trace) [C3110] handle_incoming PNRC_CLOSED_READ for server connection. read_closed:T, flow_enabled:F
       adaptor still has no place to put the 21 bytes
   TCP_ADAPTOR (debug) [C3110][L6218] Waiting for credit before initiating ingress stream message
       raw connection is write- and read-closed. it shuts down and the 21 bytes never went to tcpListener client
   TCP_ADAPTOR (info) [C3110] PN_RAW_CONNECTION_DISCONNECTED
   TCP_ADAPTOR (debug) [C3110][L6218] handle_disconnected - close outstream
   TCP_ADAPTOR (debug) [C3110][L6219] handle_disconnected - detach incoming
   TCP_ADAPTOR (debug) [C3110][L6218] handle_disconnected - detach outgoing
   TCP_ADAPTOR (debug) qdr_tcp_activate: no connection context
   TCP_ADAPTOR (debug) [C3110] qdr_add_tcp_connection_CT 127.0.0.1:9090 (2)
   TCP_ADAPTOR (debug) [C3110] qdr_del_tcp_connection_CT 127.0.0.1:9090 deleted. bytes_in=21, bytes_out=5, opened_time=28, last_in_time=28, last_out_time=28. Connections remaining 1
   ```




-- 
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

[GitHub] [qpid-dispatch] ChugR commented on pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
ChugR commented on pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#issuecomment-821506067


   Adding a self test for this now


-- 
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

[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
ChugR commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r615924379



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -630,64 +780,92 @@ static void handle_connection_event(pn_event_t *e, qd_server_t *qd_server, void
         }
     }
     case PN_RAW_CONNECTION_CLOSED_READ: {
-        qd_log(log, QD_LOG_DEBUG, "[C%"PRIu64"] PN_RAW_CONNECTION_CLOSED_READ", conn->conn_id);
-        conn->q2_blocked = false;
-        handle_incoming_impl(conn, true);
+        qd_log(log, QD_LOG_DEBUG, "[C%"PRIu64"][L%"PRIu64"] PN_RAW_CONNECTION_CLOSED_READ",
+               conn->conn_id, conn->incoming_id);
         sys_mutex_lock(conn->activation_lock);
+        conn->q2_blocked = false;
         conn->raw_closed_read = true;
         sys_mutex_unlock(conn->activation_lock);
-        pn_raw_connection_close(conn->pn_raw_conn);
+        handle_incoming(conn, "PNRC_CLOSED_READ");
         break;
     }
     case PN_RAW_CONNECTION_CLOSED_WRITE: {
-        qd_log(log, QD_LOG_DEBUG, "[C%"PRIu64"] PN_RAW_CONNECTION_CLOSED_WRITE", conn->conn_id);
+        qd_log(log, QD_LOG_DEBUG,
+               "[C%"PRIu64"] PN_RAW_CONNECTION_CLOSED_WRITE",
+               conn->conn_id);
         sys_mutex_lock(conn->activation_lock);
         conn->raw_closed_write = true;
         sys_mutex_unlock(conn->activation_lock);
-        pn_raw_connection_close(conn->pn_raw_conn);
+        if (conn->ingress) {

Review comment:
       Removing the conditional causes data loss on the connector side.
   On a connector/server connection the sequence goes:
   
   - EOS seen. Call pnrc_write_close() for connector connection
   - PNRC_CLOSED_WRITE event
   - Call pnrc_connection_close()  <-- called because conditional was removed
   - PNRC_CLOSED_READ event
   - PNRC_READ and PNRC_WRITTEN to recover buffers
   - PNRC_CONNECTION_DISCONNECTED
   
   If connection_close is not called on the connector then the write_close() propagates to the server, the server closes the connection, the connector-side of the adaptor receives the server's last data, and then CLOSED_READ comes in and the connection is shut.




-- 
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

[GitHub] [qpid-dispatch] codecov-commenter edited a comment on pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#issuecomment-822536755


   # [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#1129](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (dc55c91) into [main](https://codecov.io/gh/apache/qpid-dispatch/commit/f67e135c0cfd579272d191b0e14342c0f4cd8cc6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f67e135) will **decrease** coverage by `0.20%`.
   > The diff coverage is `79.26%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/graphs/tree.svg?width=650&height=150&src=pr&token=rk2Cgd27pP&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##             main    #1129      +/-   ##
   ==========================================
   - Coverage   84.37%   84.16%   -0.21%     
   ==========================================
     Files         111      111              
     Lines       27536    27619      +83     
   ==========================================
   + Hits        23233    23246      +13     
   - Misses       4303     4373      +70     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [src/adaptors/tcp\_adaptor.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL2FkYXB0b3JzL3RjcF9hZGFwdG9yLmM=) | `67.73% <79.26%> (-1.41%)` | :arrow_down: |
   | [src/iterator.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL2l0ZXJhdG9yLmM=) | `89.46% <0.00%> (-3.71%)` | :arrow_down: |
   | [src/router\_node.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9ub2RlLmM=) | `92.75% <0.00%> (-0.84%)` | :arrow_down: |
   | [src/router\_core/transfer.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL3RyYW5zZmVyLmM=) | `93.53% <0.00%> (-0.65%)` | :arrow_down: |
   | [src/router\_core/forwarder.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL2ZvcndhcmRlci5j) | `92.67% <0.00%> (-0.40%)` | :arrow_down: |
   | [src/router\_core/delivery.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL2RlbGl2ZXJ5LmM=) | `92.74% <0.00%> (-0.39%)` | :arrow_down: |
   | [src/adaptors/http1/http1\_server.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL2FkYXB0b3JzL2h0dHAxL2h0dHAxX3NlcnZlci5j) | `84.59% <0.00%> (-0.30%)` | :arrow_down: |
   | [src/router\_core/connections.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL2Nvbm5lY3Rpb25zLmM=) | `89.71% <0.00%> (-0.21%)` | :arrow_down: |
   | [src/server.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3NlcnZlci5j) | `86.68% <0.00%> (-0.08%)` | :arrow_down: |
   | ... and [36 more](https://codecov.io/gh/apache/qpid-dispatch/pull/1129/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f67e135...dc55c91](https://codecov.io/gh/apache/qpid-dispatch/pull/1129?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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

[GitHub] [qpid-dispatch] grs commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r614972470



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -57,11 +57,15 @@ struct qdr_tcp_connection_t {
     qdr_delivery_t       *outstream;
     bool                  ingress;
     bool                  flow_enabled;
+    bool                  incoming_started;
     bool                  egress_dispatcher;
     bool                  connector_closed;//only used if egress_dispatcher=true
     bool                  in_list;         // This connection is in the adaptor's connections list
-    bool                  raw_closed_read;
-    bool                  raw_closed_write;
+    bool                  raw_closed_read;   // proton event seen
+    bool                  raw_closed_write;  // proton event seen or write_close called
+    bool                  raw_read_shutdown; // stream closed
+    bool                  read_eos_seen;
+    qd_buffer_list_t      early_raw_read_bufs; // read from raw conn before ingress stream ready

Review comment:
       I'm not too keen on granting buffers before we are ready for them. It doesn't seem necessary to me and it complicates reasoning about the code.

##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -116,6 +120,26 @@ static inline uint64_t qdr_tcp_conn_linkid(const qdr_tcp_connection_t *conn)
     return conn->instream ? conn->incoming_id : conn->outgoing_id;
 }
 
+static inline const char * qdr_link_direction_name(const qdr_link_t *link)
+{
+    assert(link);
+    return qdr_link_direction(link) == QD_OUTGOING ? "outgoing" : "incoming";
+}
+
+static inline const char * qdr_tcp_connection_role_name(const qdr_tcp_connection_t *tc)
+{
+    assert(tc);
+    return tc->ingress ? "client" : "server";

Review comment:
       I find the use of client and server confusing here. What you call the client is actually the server side in tcp terms. I think ingress/egress is less ambiguous (perhaps qualified by bridge, e.g. 'ingress bridge') and is already used elsewhere even with this patch.

##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -782,6 +965,14 @@ static void qdr_tcp_open_server_side_connection(qdr_tcp_connection_t* tc)
 
     // This attach passes the ownership of the delivery from the core-side connection and link
     // to the adaptor-side outgoing connection and link.
+    uint64_t i_conn_id = 0;
+    uint64_t i_link_id = 0;
+    uint64_t i_dlv_id = 0;
+    if (!!tc->initial_delivery) {
+        i_conn_id = tc->initial_delivery->conn_id;

Review comment:
       Minor point, but why are these initialised in a separate conditional block up here, when they seem only to be used in the subsequent block for the identical condition and are not used in the qdr_link_first_attach() call that is the only thing separating these blocks? Makes it easier to read if we just declared these just before use I think.

##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -1257,13 +1489,17 @@ static void qdr_tcp_delivery_update(void *context, qdr_delivery_t *dlv, uint64_t
     void* link_context = qdr_link_get_context(qdr_delivery_link(dlv));
     if (link_context) {
         qdr_tcp_connection_t* tc = (qdr_tcp_connection_t*) link_context;
-        qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG, DLV_FMT" qdr_tcp_delivery_update: disp: %"PRIu64", settled: %s",
+        qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG,
+               DLV_FMT" qdr_tcp_delivery_update: disp: %"PRIu64", settled: %s",
                DLV_ARGS(dlv), disp, settled ? "true" : "false");
 
         //
         // If one of the streaming deliveries is ever settled, the connection must be torn down.

Review comment:
       I think now that we have separate close calls for read and write sides, we should close for read if the instream is settled and close for write when the outstream is settled.
   
   If we then settle and complete the instream on CLOSED_READ and settle the outstream on CLOSED_WRITE, we are then conveying more accurately what each tcp endpoint does to its peer. If the instream has not been started when we get CLOSED_READ on ingress, we can close the connection entirely (on egress we can just send an empty reply). 

##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -630,64 +780,92 @@ static void handle_connection_event(pn_event_t *e, qd_server_t *qd_server, void
         }
     }
     case PN_RAW_CONNECTION_CLOSED_READ: {
-        qd_log(log, QD_LOG_DEBUG, "[C%"PRIu64"] PN_RAW_CONNECTION_CLOSED_READ", conn->conn_id);
-        conn->q2_blocked = false;
-        handle_incoming_impl(conn, true);
+        qd_log(log, QD_LOG_DEBUG, "[C%"PRIu64"][L%"PRIu64"] PN_RAW_CONNECTION_CLOSED_READ",
+               conn->conn_id, conn->incoming_id);
         sys_mutex_lock(conn->activation_lock);
+        conn->q2_blocked = false;
         conn->raw_closed_read = true;
         sys_mutex_unlock(conn->activation_lock);
-        pn_raw_connection_close(conn->pn_raw_conn);
+        handle_incoming(conn, "PNRC_CLOSED_READ");
         break;
     }
     case PN_RAW_CONNECTION_CLOSED_WRITE: {
-        qd_log(log, QD_LOG_DEBUG, "[C%"PRIu64"] PN_RAW_CONNECTION_CLOSED_WRITE", conn->conn_id);
+        qd_log(log, QD_LOG_DEBUG,
+               "[C%"PRIu64"] PN_RAW_CONNECTION_CLOSED_WRITE",
+               conn->conn_id);
         sys_mutex_lock(conn->activation_lock);
         conn->raw_closed_write = true;
         sys_mutex_unlock(conn->activation_lock);
-        pn_raw_connection_close(conn->pn_raw_conn);
+        if (conn->ingress) {

Review comment:
       Why only on ingress?
   
   I would expect that on CLOSED_WRITE we settle the incoming qdr delivery, i.e. outstream (since we can no longer forward any data received). That signals the peer that they should close their socket for read.




-- 
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

[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
ChugR commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r615061225



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -1257,13 +1489,17 @@ static void qdr_tcp_delivery_update(void *context, qdr_delivery_t *dlv, uint64_t
     void* link_context = qdr_link_get_context(qdr_delivery_link(dlv));
     if (link_context) {
         qdr_tcp_connection_t* tc = (qdr_tcp_connection_t*) link_context;
-        qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG, DLV_FMT" qdr_tcp_delivery_update: disp: %"PRIu64", settled: %s",
+        qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG,
+               DLV_FMT" qdr_tcp_delivery_update: disp: %"PRIu64", settled: %s",
                DLV_ARGS(dlv), disp, settled ? "true" : "false");
 
         //
         // If one of the streaming deliveries is ever settled, the connection must be torn down.

Review comment:
       I disagree. This code patch is trying to fix DISPATCH-1878 where a client sends data and closes the connection. The expectation is that the data gets sent over the wire to where it's going and the propagates the close after that. Any data will sent by the server will come back to the client. Then when the server closes that close will be propagated to the client.
   A tcpListener READ_CLOSED event must be propagated to the peer tcpConnector as a write_close() control. Then the connector READ_CLOSE event becomes the listener write_close() control.
   Short circuiting that control flow will lose data.




-- 
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

[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
ChugR commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r615052175



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -630,64 +780,92 @@ static void handle_connection_event(pn_event_t *e, qd_server_t *qd_server, void
         }
     }
     case PN_RAW_CONNECTION_CLOSED_READ: {
-        qd_log(log, QD_LOG_DEBUG, "[C%"PRIu64"] PN_RAW_CONNECTION_CLOSED_READ", conn->conn_id);
-        conn->q2_blocked = false;
-        handle_incoming_impl(conn, true);
+        qd_log(log, QD_LOG_DEBUG, "[C%"PRIu64"][L%"PRIu64"] PN_RAW_CONNECTION_CLOSED_READ",
+               conn->conn_id, conn->incoming_id);
         sys_mutex_lock(conn->activation_lock);
+        conn->q2_blocked = false;
         conn->raw_closed_read = true;
         sys_mutex_unlock(conn->activation_lock);
-        pn_raw_connection_close(conn->pn_raw_conn);
+        handle_incoming(conn, "PNRC_CLOSED_READ");
         break;
     }
     case PN_RAW_CONNECTION_CLOSED_WRITE: {
-        qd_log(log, QD_LOG_DEBUG, "[C%"PRIu64"] PN_RAW_CONNECTION_CLOSED_WRITE", conn->conn_id);
+        qd_log(log, QD_LOG_DEBUG,
+               "[C%"PRIu64"] PN_RAW_CONNECTION_CLOSED_WRITE",
+               conn->conn_id);
         sys_mutex_lock(conn->activation_lock);
         conn->raw_closed_write = true;
         sys_mutex_unlock(conn->activation_lock);
-        pn_raw_connection_close(conn->pn_raw_conn);
+        if (conn->ingress) {

Review comment:
       I will try removing that conditiona.




-- 
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

[GitHub] [qpid-dispatch] grs commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r615601130



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -1257,13 +1489,17 @@ static void qdr_tcp_delivery_update(void *context, qdr_delivery_t *dlv, uint64_t
     void* link_context = qdr_link_get_context(qdr_delivery_link(dlv));
     if (link_context) {
         qdr_tcp_connection_t* tc = (qdr_tcp_connection_t*) link_context;
-        qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG, DLV_FMT" qdr_tcp_delivery_update: disp: %"PRIu64", settled: %s",
+        qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG,
+               DLV_FMT" qdr_tcp_delivery_update: disp: %"PRIu64", settled: %s",
                DLV_ARGS(dlv), disp, settled ? "true" : "false");
 
         //
         // If one of the streaming deliveries is ever settled, the connection must be torn down.

Review comment:
       Readiness to read will always be a per-connection setup. 




-- 
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

[GitHub] [qpid-dispatch] grs commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE

Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #1129:
URL: https://github.com/apache/qpid-dispatch/pull/1129#discussion_r618302065



##########
File path: src/adaptors/tcp_adaptor.c
##########
@@ -1257,13 +1489,17 @@ static void qdr_tcp_delivery_update(void *context, qdr_delivery_t *dlv, uint64_t
     void* link_context = qdr_link_get_context(qdr_delivery_link(dlv));
     if (link_context) {
         qdr_tcp_connection_t* tc = (qdr_tcp_connection_t*) link_context;
-        qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG, DLV_FMT" qdr_tcp_delivery_update: disp: %"PRIu64", settled: %s",
+        qd_log(tcp_adaptor->log_source, QD_LOG_DEBUG,
+               DLV_FMT" qdr_tcp_delivery_update: disp: %"PRIu64", settled: %s",
                DLV_ARGS(dlv), disp, settled ? "true" : "false");
 
         //
         // If one of the streaming deliveries is ever settled, the connection must be torn down.

Review comment:
       Update on this. I was wrong. We don't actually explicitly settle deliveries except in the case where the connector cannot connect (i.e. disconnects before connecting), in which case the 'initial-delivery' is settled. Deliveries are implicitly settled if there is any failure. We convey the half clsoed state solely through completing the message (i.e. the EOS is the only trigger).
   
   I've pushed a fix that handles the case where there is no server. Also tested router failures, including middle router in a three-way chain with tcp-listener and tcp-connector at opposite ends.




-- 
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