From df13f8e8c2199b572f19d295e5c59e7502204b3a Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Tue, 20 May 2014 10:32:23 +0200 Subject: [PATCH] bits.close: introduce connection close tracking Make all code use connclose() and connkeep() when changing the "close state" for a connection. These two macros take a string argument with an explanation, and debug builds of curl will include that in the debug output. Helps tracking connection re-use/close issues. --- lib/asyn-ares.c | 4 ++-- lib/asyn-thread.c | 2 +- lib/connect.c | 17 +++++++++++++++++ lib/connect.h | 19 ++++++++++++++++++- lib/ftp.c | 14 +++++++------- lib/http.c | 25 +++++++++++++------------ lib/http_proxy.c | 2 +- lib/imap.c | 4 ++-- lib/ldap.c | 2 +- lib/multi.c | 10 +++++----- lib/openldap.c | 7 ++++--- lib/pop3.c | 4 ++-- lib/smtp.c | 4 ++-- lib/ssh.c | 4 ++-- lib/tftp.c | 6 +++--- lib/transfer.c | 6 +++--- lib/url.c | 2 +- lib/urldata.h | 1 + 18 files changed, 85 insertions(+), 48 deletions(-) diff --git a/lib/asyn-ares.c b/lib/asyn-ares.c index 94ee767..d651c25 100644 --- a/lib/asyn-ares.c +++ b/lib/asyn-ares.c @@ -5,7 +5,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2013, Daniel Stenberg, , et al. + * Copyright (C) 1998 - 2014, Daniel Stenberg, , et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -421,7 +421,7 @@ CURLcode Curl_resolver_wait_resolv(struct connectdata *conn, cleaning up this connection properly. TODO: remove this action from here, it is not a name resolver decision. */ - conn->bits.close = TRUE; + connclose(conn, "c-ares resolve failed"); return rc; } diff --git a/lib/asyn-thread.c b/lib/asyn-thread.c index c72c064..a0f4366 100644 --- a/lib/asyn-thread.c +++ b/lib/asyn-thread.c @@ -446,7 +446,7 @@ CURLcode Curl_resolver_wait_resolv(struct connectdata *conn, destroy_async_data(&conn->async); if(!conn->async.dns) - conn->bits.close = TRUE; + connclose(conn, "asynch resolve failed"); return (rc); } diff --git a/lib/connect.c b/lib/connect.c index ca6e346..de78c33 100644 --- a/lib/connect.c +++ b/lib/connect.c @@ -1321,3 +1321,20 @@ CURLcode Curl_socket(struct connectdata *conn, return CURLE_OK; } + +#ifdef CURLDEBUG +/* + * Curl_conncontrol() is used to set the conn->bits.close bit on or off. It + * MUST be called with the connclose() or connclose() macros with a stated + * reason. The reason is only shown in debug builds but helps to figure out + * decision paths when connections are or aren't re-used as expected. + */ +void Curl_conncontrol(struct connectdata *conn, bool closeit, + const char *reason) +{ + infof(conn->data, "Marked for [%s]: %s\n", closeit?"closure":"keep alive", + reason); + conn->bits.close = closeit; /* the only place in the source code that should + assign this bit */ +} +#endif diff --git a/lib/connect.h b/lib/connect.h index 89653f7..7bc391b 100644 --- a/lib/connect.h +++ b/lib/connect.h @@ -7,7 +7,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 1998 - 2013, Daniel Stenberg, , et al. + * Copyright (C) 1998 - 2014, Daniel Stenberg, , et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -102,4 +102,21 @@ CURLcode Curl_socket(struct connectdata *conn, struct Curl_sockaddr_ex *addr, curl_socket_t *sockfd); +#ifdef CURLDEBUG +/* + * Curl_connclose() sets the bit.close bit to TRUE with an explanation. + * Nothing else. + */ +void Curl_conncontrol(struct connectdata *conn, + bool closeit, + const char *reason); +#define connclose(x,y) Curl_conncontrol(x,TRUE, y) +#define connkeep(x,y) Curl_conncontrol(x, FALSE, y) +#else /* if !CURLDEBUG */ + +#define connclose(x,y) (x)->bits.close = TRUE +#define connkeep(x,y) (x)->bits.close = FALSE + +#endif + #endif /* HEADER_CURL_CONNECT_H */ diff --git a/lib/ftp.c b/lib/ftp.c index cadec10..4c4396a 100644 --- a/lib/ftp.c +++ b/lib/ftp.c @@ -3164,7 +3164,7 @@ static CURLcode ftp_connect(struct connectdata *conn, *done = FALSE; /* default to not done yet */ /* We always support persistent connections on ftp */ - conn->bits.close = FALSE; + connkeep(conn, "FTP default"); pp->response_time = RESP_TIMEOUT; /* set default response time-out */ pp->statemach_act = ftp_statemach_act; @@ -3248,7 +3248,7 @@ static CURLcode ftp_done(struct connectdata *conn, CURLcode status, ftpc->ctl_valid = FALSE; ftpc->cwdfail = TRUE; /* set this TRUE to prevent us to remember the current path, as this connection is going */ - conn->bits.close = TRUE; /* marked for closure */ + connclose(conn, "FTP ended with bad error code"); result = status; /* use the already set error code */ break; } @@ -3272,7 +3272,7 @@ static CURLcode ftp_done(struct connectdata *conn, CURLcode status, if(!result) result = CURLE_OUT_OF_MEMORY; ftpc->ctl_valid = FALSE; /* mark control connection as bad */ - conn->bits.close = TRUE; /* mark for connection closure */ + connclose(conn, "FTP: out of memory!"); /* mark for connection closure */ ftpc->prevpath = NULL; /* no path remembering */ } else { @@ -3315,7 +3315,7 @@ static CURLcode ftp_done(struct connectdata *conn, CURLcode status, failf(data, "Failure sending ABOR command: %s", curl_easy_strerror(result)); ftpc->ctl_valid = FALSE; /* mark control connection as bad */ - conn->bits.close = TRUE; /* mark for connection closure */ + connclose(conn, "ABOR command failed"); /* connection closure */ } } @@ -3354,7 +3354,7 @@ static CURLcode ftp_done(struct connectdata *conn, CURLcode status, if(!nread && (CURLE_OPERATION_TIMEDOUT == result)) { failf(data, "control connection looks dead"); ftpc->ctl_valid = FALSE; /* mark control connection as bad */ - conn->bits.close = TRUE; /* mark for closure */ + connclose(conn, "Timeout or similar in FTP DONE operation"); /* close */ } if(result) @@ -3364,7 +3364,7 @@ static CURLcode ftp_done(struct connectdata *conn, CURLcode status, /* we have just sent ABOR and there is no reliable way to check if it was * successful or not; we have to close the connection now */ infof(data, "partial download completed, closing connection\n"); - conn->bits.close = TRUE; /* mark for closure */ + connclose(conn, "Partial download with no ability to check"); return result; } @@ -4133,7 +4133,7 @@ static CURLcode ftp_quit(struct connectdata *conn) failf(conn->data, "Failure sending QUIT command: %s", curl_easy_strerror(result)); conn->proto.ftpc.ctl_valid = FALSE; /* mark control connection as bad */ - conn->bits.close = TRUE; /* mark for connection closure */ + connclose(conn, "QUIT command failed"); /* mark for connection closure */ state(conn, FTP_STOP); return result; } diff --git a/lib/http.c b/lib/http.c index 7dde821..cfdaadd 100644 --- a/lib/http.c +++ b/lib/http.c @@ -76,6 +76,7 @@ #include "bundles.h" #include "pipeline.h" #include "http2.h" +#include "connect.h" #define _MPRINTF_REPLACE /* use our functions only */ #include @@ -449,7 +450,7 @@ static CURLcode http_perhapsrewind(struct connectdata *conn) /* This is not NTLM or many bytes left to send: close */ - conn->bits.close = TRUE; + connclose(conn, "Mid-auth HTTP and much data left to send"); data->req.size = 0; /* don't download any more than 0 bytes */ /* There still is data left to send, but this connection is marked for @@ -1337,7 +1338,7 @@ CURLcode Curl_http_connect(struct connectdata *conn, bool *done) /* We default to persistent connections. We set this already in this connect function to make the re-use checks properly be able to check this bit. */ - conn->bits.close = FALSE; + connkeep(conn, "HTTP default"); /* the CONNECT procedure might not have been completed */ result = Curl_proxy_connect(conn); @@ -1382,8 +1383,8 @@ static CURLcode https_connecting(struct connectdata *conn, bool *done) /* perform SSL initialization for this socket */ result = Curl_ssl_connect_nonblocking(conn, FIRSTSOCKET, done); if(result) - conn->bits.close = TRUE; /* a failed connection is marked for closure - to prevent (bad) re-use or similar */ + connclose(conn, "Failed HTTPS connection"); + return result; } #endif @@ -3016,7 +3017,7 @@ CURLcode Curl_http_readwrite_headers(struct SessionHandle *data, signal the end of the document. */ infof(data, "no chunk, no close, no size. Assume close to " "signal end\n"); - conn->bits.close = TRUE; + connclose(conn, "HTTP: No end-of-message indicator"); } } @@ -3085,7 +3086,7 @@ CURLcode Curl_http_readwrite_headers(struct SessionHandle *data, */ if(!k->upload_done) { infof(data, "HTTP error before end of send, stop sending\n"); - conn->bits.close = TRUE; /* close after this */ + connclose(conn, "Stop sending data before everything sent"); k->upload_done = TRUE; k->keepon &= ~KEEP_SEND; /* don't send */ if(data->state.expect100header) @@ -3280,7 +3281,7 @@ CURLcode Curl_http_readwrite_headers(struct SessionHandle *data, we get one of those fancy headers that tell us the server keeps it open for us! */ infof(data, "HTTP 1.0, assume close after body\n"); - conn->bits.close = TRUE; + connclose(conn, "HTTP/1.0 close after body"); } else if(conn->httpversion >= 11 && !conn->bits.close) { @@ -3355,7 +3356,7 @@ CURLcode Curl_http_readwrite_headers(struct SessionHandle *data, /* Negative Content-Length is really odd, and we know it happens for example when older Apache servers send large files */ - conn->bits.close = TRUE; + connclose(conn, "negative content-length"); infof(data, "Negative content-length: %" CURL_FORMAT_CURL_OFF_T ", closing after transfer\n", contentlength); } @@ -3393,7 +3394,7 @@ CURLcode Curl_http_readwrite_headers(struct SessionHandle *data, * connection will be kept alive for our pleasure. * Default action for 1.0 is to close. */ - conn->bits.close = FALSE; /* don't close when done */ + connkeep(conn, "Proxy-Connection keep-alive"); /* don't close */ infof(data, "HTTP/1.0 proxy connection set to keep alive!\n"); } else if((conn->httpversion == 11) && @@ -3404,7 +3405,7 @@ CURLcode Curl_http_readwrite_headers(struct SessionHandle *data, * We get a HTTP/1.1 response from a proxy and it says it'll * close down after this transfer. */ - conn->bits.close = TRUE; /* close when done */ + connclose(conn, "Proxy-Connection: asked to close after done"); infof(data, "HTTP/1.1 proxy connection set close!\n"); } else if((conn->httpversion == 10) && @@ -3415,7 +3416,7 @@ CURLcode Curl_http_readwrite_headers(struct SessionHandle *data, * pleasure. Default action for 1.0 is to close. * * [RFC2068, section 19.7.1] */ - conn->bits.close = FALSE; /* don't close when done */ + connkeep(conn, "Connection keep-alive"); infof(data, "HTTP/1.0 connection set to keep alive!\n"); } else if(Curl_compareheader(k->p, "Connection:", "close")) { @@ -3425,7 +3426,7 @@ CURLcode Curl_http_readwrite_headers(struct SessionHandle *data, * the connection will close when this request has been * served. */ - conn->bits.close = TRUE; /* close when done */ + connclose(conn, "Connection: close used"); } else if(checkprefix("Transfer-Encoding:", k->p)) { /* One or more encodings. We check for chunked and/or a compression diff --git a/lib/http_proxy.c b/lib/http_proxy.c index 0ee1a73..a98c68c 100644 --- a/lib/http_proxy.c +++ b/lib/http_proxy.c @@ -69,7 +69,7 @@ CURLcode Curl_proxy_connect(struct connectdata *conn) prot_save = conn->data->req.protop; memset(&http_proxy, 0, sizeof(http_proxy)); conn->data->req.protop = &http_proxy; - conn->bits.close = FALSE; + connkeep(conn, "HTTP proxy CONNECT"); result = Curl_proxyCONNECT(conn, FIRSTSOCKET, conn->host.name, conn->remote_port); conn->data->req.protop = prot_save; diff --git a/lib/imap.c b/lib/imap.c index 7130603..0570ecc 100644 --- a/lib/imap.c +++ b/lib/imap.c @@ -1881,7 +1881,7 @@ static CURLcode imap_connect(struct connectdata *conn, bool *done) *done = FALSE; /* default to not done yet */ /* We always support persistent connections in IMAP */ - conn->bits.close = FALSE; + connkeep(conn, "IMAP default"); /* Set the default response time-out */ pp->response_time = RESP_TIMEOUT; @@ -1938,7 +1938,7 @@ static CURLcode imap_done(struct connectdata *conn, CURLcode status, return CURLE_OK; if(status) { - conn->bits.close = TRUE; /* marked for closure */ + connclose(conn, "IMAP done with bad status"); /* marked for closure */ result = status; /* use the already set error code */ } else if(!data->set.connect_only && !imap->custom && diff --git a/lib/ldap.c b/lib/ldap.c index 4c98789..bbeeb6e 100644 --- a/lib/ldap.c +++ b/lib/ldap.c @@ -466,7 +466,7 @@ quit: /* no data to transfer */ Curl_setup_transfer(conn, -1, -1, FALSE, NULL, -1, NULL); - conn->bits.close = TRUE; + connclose(conn, "LDAP connection always disable re-use"); return status; } diff --git a/lib/multi.c b/lib/multi.c index 72fde74..1fb341c 100644 --- a/lib/multi.c +++ b/lib/multi.c @@ -515,7 +515,7 @@ CURLMcode curl_multi_remove_handle(CURLM *multi_handle, /* If the handle is in a pipeline and has started sending off its request but not received its response yet, we need to close connection. */ - data->easy_conn->bits.close = TRUE; + connclose(data->easy_conn, "Removed with partial response"); /* Set connection owner so that Curl_done() closes it. We can sefely do this here since connection is killed. */ data->easy_conn->data = easy; @@ -1011,7 +1011,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, /* Force the connection closed because the server could continue to send us stuff at any time. (The disconnect_conn logic used below doesn't work at this point). */ - data->easy_conn->bits.close = TRUE; + connclose(data->easy_conn, "Disconnected with pending data"); data->result = CURLE_OPERATION_TIMEDOUT; multistate(data, CURLM_STATE_COMPLETED); break; @@ -1239,7 +1239,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, case CURLM_STATE_DO: if(data->set.connect_only) { /* keep connection open for application to use the socket */ - data->easy_conn->bits.close = FALSE; + connkeep(data->easy_conn, "CONNECT_ONLY"); multistate(data, CURLM_STATE_DONE); data->result = CURLE_OK; result = CURLM_CALL_MULTI_PERFORM; @@ -1525,7 +1525,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, */ if(!(data->easy_conn->handler->flags & PROTOPT_DUAL)) - data->easy_conn->bits.close = TRUE; + connclose(data->easy_conn, "Transfer returned error"); Curl_posttransfer(data); Curl_done(&data->easy_conn, data->result, FALSE); @@ -1705,7 +1705,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, /* aborted due to progress callback return code must close the connection */ data->result = CURLE_ABORTED_BY_CALLBACK; - data->easy_conn->bits.close = TRUE; + connclose(data->easy_conn, "Aborted by callback"); /* if not yet in DONE state, go there, otherwise COMPLETED */ multistate(data, (data->mstate < CURLM_STATE_DONE)? diff --git a/lib/openldap.c b/lib/openldap.c index 2af14ba..df8d938 100644 --- a/lib/openldap.c +++ b/lib/openldap.c @@ -6,7 +6,7 @@ * \___|\___/|_| \_\_____| * * Copyright (C) 2010, 2013, Howard Chu, - * Copyright (C) 2011 - 2013, Daniel Stenberg, , et al. + * Copyright (C) 2011 - 2014, Daniel Stenberg, , et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -46,6 +46,7 @@ #include "curl_ldap.h" #include "curl_memory.h" #include "curl_base64.h" +#include "connect.h" #define _MPRINTF_REPLACE /* use our functions only */ #include @@ -175,7 +176,7 @@ static CURLcode ldap_setup(struct connectdata *conn) return CURLE_OUT_OF_MEMORY; li->proto = proto; conn->proto.generic = li; - conn->bits.close = FALSE; + connkeep(conn, "OpenLDAP default"); /* TODO: * - provide option to choose SASL Binds instead of Simple */ @@ -349,7 +350,7 @@ static CURLcode ldap_do(struct connectdata *conn, bool *done) int msgid; struct SessionHandle *data=conn->data; - conn->bits.close = FALSE; + connkeep(conn, "OpenLDAP do"); infof(data, "LDAP local: %s\n", data->change.url); diff --git a/lib/pop3.c b/lib/pop3.c index 964804b..314567e 100644 --- a/lib/pop3.c +++ b/lib/pop3.c @@ -1547,7 +1547,7 @@ static CURLcode pop3_connect(struct connectdata *conn, bool *done) *done = FALSE; /* default to not done yet */ /* We always support persistent connections in POP3 */ - conn->bits.close = FALSE; + connkeep(conn, "POP3 default"); /* Set the default response time-out */ pp->response_time = RESP_TIMEOUT; @@ -1601,7 +1601,7 @@ static CURLcode pop3_done(struct connectdata *conn, CURLcode status, return CURLE_OK; if(status) { - conn->bits.close = TRUE; /* marked for closure */ + connclose(conn, "POP3 done with bad status"); result = status; /* use the already set error code */ } diff --git a/lib/smtp.c b/lib/smtp.c index 762a8d1..5938c3f 100644 --- a/lib/smtp.c +++ b/lib/smtp.c @@ -1588,7 +1588,7 @@ static CURLcode smtp_connect(struct connectdata *conn, bool *done) *done = FALSE; /* default to not done yet */ /* We always support persistent connections in SMTP */ - conn->bits.close = FALSE; + connkeep(conn, "SMTP default"); /* Set the default response time-out */ pp->response_time = RESP_TIMEOUT; @@ -1650,7 +1650,7 @@ static CURLcode smtp_done(struct connectdata *conn, CURLcode status, return CURLE_OK; if(status) { - conn->bits.close = TRUE; /* marked for closure */ + connclose(conn, "SMTP done with bad status"); /* marked for closure */ result = status; /* use the already set error code */ } else if(!data->set.connect_only && data->set.upload && data->set.mail_rcpt) { diff --git a/lib/ssh.c b/lib/ssh.c index 5e18ca7..b248b43 100644 --- a/lib/ssh.c +++ b/lib/ssh.c @@ -2544,7 +2544,7 @@ static CURLcode ssh_statemach_act(struct connectdata *conn, bool *block) memset(sshc, 0, sizeof(struct ssh_conn)); - conn->bits.close = TRUE; + connclose(conn, "SSH session free"); sshc->state = SSH_SESSION_FREE; /* current */ sshc->nextstate = SSH_NO_STATE; state(conn, SSH_STOP); @@ -2747,7 +2747,7 @@ static CURLcode ssh_connect(struct connectdata *conn, bool *done) /* We default to persistent connections. We set this already in this connect function to make the re-use checks properly be able to check this bit. */ - conn->bits.close = FALSE; + connkeep(conn, "SSH default"); if(conn->handler->protocol & CURLPROTO_SCP) { conn->recv[FIRSTSOCKET] = scp_recv; diff --git a/lib/tftp.c b/lib/tftp.c index d19f480..e499c45 100644 --- a/lib/tftp.c +++ b/lib/tftp.c @@ -974,9 +974,9 @@ static CURLcode tftp_connect(struct connectdata *conn, bool *done) return CURLE_OUT_OF_MEMORY; } - conn->bits.close = TRUE; /* we don't keep TFTP connections up bascially - because there's none or very little gain for UDP - */ + /* we don't keep TFTP connections up bascially because there's none or very + * little gain for UDP */ + connclose(conn, "TFTP"); state->conn = conn; state->sockfd = state->conn->sock[FIRSTSOCKET]; diff --git a/lib/transfer.c b/lib/transfer.c index c1cdde6..e0f613b 100644 --- a/lib/transfer.c +++ b/lib/transfer.c @@ -569,7 +569,7 @@ static CURLcode readwrite_data(struct SessionHandle *data, infof(data, "Simulate a HTTP 304 response!\n"); /* we abort the transfer before it is completed == we ruin the re-use ability. Close the connection */ - conn->bits.close = TRUE; + connclose(conn, "Simulated 304 handling"); return CURLE_OK; } } /* we have a time condition */ @@ -1818,7 +1818,7 @@ Curl_reconnect_request(struct connectdata **connp) infof(data, "Re-used connection seems dead, get a new one\n"); - conn->bits.close = TRUE; /* enforce close of this connection */ + connclose(conn, "Reconnect dead connection"); /* enforce close */ result = Curl_done(&conn, result, FALSE); /* we are so done with this */ /* conn may no longer be a good pointer, clear it to avoid mistakes by @@ -1891,7 +1891,7 @@ CURLcode Curl_retry_request(struct connectdata *conn, if(!*url) return CURLE_OUT_OF_MEMORY; - conn->bits.close = TRUE; /* close this connection */ + connclose(conn, "retry"); /* close this connection */ conn->bits.retry = TRUE; /* mark this as a connection we're about to retry. Marking it this way should prevent i.e HTTP transfers to return diff --git a/lib/url.c b/lib/url.c index b0aade1..7d2592b 100644 --- a/lib/url.c +++ b/lib/url.c @@ -3590,7 +3590,7 @@ static struct connectdata *allocate_conn(struct SessionHandle *data) /* Default protocol-independent behavior doesn't support persistent connections, so we set this to force-close. Protocols that support this need to set this to FALSE in their "curl_do" functions. */ - conn->bits.close = TRUE; + connclose(conn, "Default to force-close"); /* Store creation time to help future close decision making */ conn->created = Curl_tvnow(); diff --git a/lib/urldata.h b/lib/urldata.h index 640cbb1..6481876 100644 --- a/lib/urldata.h +++ b/lib/urldata.h @@ -475,6 +475,7 @@ struct negotiatedata { * Boolean values that concerns this connection. */ struct ConnectBits { + /* always modify bits.close with the connclose() and connkeep() macros! */ bool close; /* if set, we close the connection after this request */ bool reuse; /* if set, this is a re-used connection */ bool proxy; /* if set, this transfer is done through a proxy - any type */ -- 2.7.4