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/16 16:53:51 UTC
[GitHub] [qpid-dispatch] grs commented on a change in pull request #1129: DISPATCH-1878: Handle half-closed TCP connections - DO NOT MERGE
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