moparisthebest/curl
1
0
Fork 0
mirror of https://github.com/moparisthebest/curl synced 2024-12-21 15:48:49 -05:00
Code Issues Releases Wiki Activity

http2: make sure stream errors don't needlessly close the connection

Browse Source 
With HTTP/2 each transfer is made in an indivial logical stream over the
connection, making most previous errors that caused the connection to get
forced-closed now instead just kill the stream and not the connection.

Fixes #941
This commit is contained in:
Daniel Stenberg 2016-08-11 14:00:23 +02:00
parent a6ddd6555e
commit 3533def3d5
11 changed files with 152 additions and 88 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
docs/KNOWN_BUGS
View File

@ -63,6 +63,7 @@ problems may have been fixed or changed somewhat since this was written!
7.5 ASCII FTP
7.6 FTP with NULs in URL parts
7.7 FTP and empty path parts in the URL
7.8 Premature transfer end but healthy control channel
8. TELNET
8.1 TELNET and time limtiations don't work
@ -441,6 +442,17 @@ problems may have been fixed or changed somewhat since this was written!
the user wants to reach the root dir (this exception SHALL remain even when
this bug is fixed).
7.8 Premature transfer end but healthy control channel
When 'multi_done' is called before the transfer has been completed the normal
way, it is considered a "premature" transfer end. In this situation, libcurl
closes the connection assuming it doesn't know the state of the connection so
it can't be reused for subsequent requests.
With FTP however, this isn't necessarily true but there are a bunch of
situations (listed in the ftp_done code) where it *could* keep the connection
alive even in this situation - but the current code doesn't. Fixing this would
allow libcurl to reuse FTP connections better.
8. TELNET

31
lib/connect.c
View File

@ -1371,25 +1371,26 @@ CURLcode Curl_socket(struct connectdata *conn,
}
#ifdef CURLDEBUG
/*
* Curl_conncontrol() is used to set the conn->bits.close bit on or off. It
* MUST be called with the connclose() or connkeep() 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.
* Curl_conncontrol() marks streams or connection for closure.
*/
void Curl_conncontrol(struct connectdata *conn, bool closeit,
const char *reason)
{
#if defined(CURL_DISABLE_VERBOSE_STRINGS)
(void) reason;
void Curl_conncontrol(struct connectdata *conn,
int ctrl /* see defines in header */
#ifdef CURLDEBUG
, const char *reason
#endif
if(closeit != conn->bits.close) {
infof(conn->data, "Marked for [%s]: %s\n", closeit?"closure":"keep alive",
reason);
)
{
/* close if a connection, or a stream that isn't multiplexed */
bool closeit = (ctrl == CONNCTRL_CONNECTION) ||
((ctrl == CONNCTRL_STREAM) && !(conn->handler->flags & PROTOPT_STREAM));
if((ctrl == CONNCTRL_STREAM) &&
(conn->handler->flags & PROTOPT_STREAM))
DEBUGF(infof(conn->data, "Kill stream: %s\n", reason));
else if(closeit != conn->bits.close) {
DEBUGF(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

40
lib/connect.h
View File

@ -7,7 +7,7 @@
* | (__| |_| | _ <| |___
* \___|\___/|_| \_\_____|
*
* Copyright (C) 1998 - 2015, Daniel Stenberg, <daniel@haxx.se>, et al.
* Copyright (C) 1998 - 2016, Daniel Stenberg, <daniel@haxx.se>, et al.
*
* This software is licensed as described in the file COPYING, which
* you should have received as part of this distribution. The terms
@ -104,21 +104,37 @@ CURLcode Curl_socket(struct connectdata *conn,
void Curl_tcpnodelay(struct connectdata *conn, curl_socket_t sockfd);
#ifdef CURLDEBUG
/*
* Curl_connclose() sets the bit.close bit to TRUE with an explanation.
* Nothing else.
* Curl_conncontrol() marks the end of a connection/stream. The 'closeit'
* argument specifies if it is the end of a connection or a stream.
*
* For stream-based protocols (such as HTTP/2), a stream close will not cause
* a connection close. Other protocols will close the connection for both
* cases.
*
* It sets the bit.close bit to TRUE (with an explanation for debug builds),
* when the connection will close.
*/
#define CONNCTRL_KEEP 0 /* undo a marked closure */
#define CONNCTRL_CONNECTION 1
#define CONNCTRL_STREAM 2
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)
int closeit
#ifdef CURLDEBUG
, const char *reason
#endif
);
#ifdef CURLDEBUG
#define streamclose(x,y) Curl_conncontrol(x, CONNCTRL_STREAM, y)
#define connclose(x,y) Curl_conncontrol(x, CONNCTRL_CONNECTION, y)
#define connkeep(x,y) Curl_conncontrol(x, CONNCTRL_KEEP, y)
#else /* if !CURLDEBUG */
#define connclose(x,y) (x)->bits.close = TRUE
#define connkeep(x,y) (x)->bits.close = FALSE
#define streamclose(x,y) Curl_conncontrol(x, CONNCTRL_STREAM)
#define connclose(x,y) Curl_conncontrol(x, CONNCTRL_CONNECTION)
#define connkeep(x,y) Curl_conncontrol(x, CONNCTRL_KEEP)
#endif
#endif /* HEADER_CURL_CONNECT_H */

39
lib/http.c
View File

@ -462,7 +462,7 @@ static CURLcode http_perhapsrewind(struct connectdata *conn)
#endif
/* This is not NTLM or many bytes left to send: close */
connclose(conn, "Mid-auth HTTP and much data left to send");
streamclose(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
@ -1452,9 +1452,8 @@ CURLcode Curl_http_done(struct connectdata *conn,
{
struct Curl_easy *data = conn->data;
struct HTTP *http = data->req.protop;
#ifdef USE_NGHTTP2
struct http_conn *httpc = &conn->proto.httpc;
#endif
infof(data, "Curl_http_done: called premature == %d\n", premature);
Curl_unencode_cleanup(conn);
@ -1467,7 +1466,7 @@ CURLcode Curl_http_done(struct connectdata *conn,
* Do not close CONNECT_ONLY connections. */
if((data->req.httpcode != 401) && (data->req.httpcode != 407) &&
!data->set.connect_only)
connclose(conn, "Negotiate transfer completed");
streamclose(conn, "Negotiate transfer completed");
Curl_cleanup_negotiate(data);
}
#endif
@ -1484,27 +1483,7 @@ CURLcode Curl_http_done(struct connectdata *conn,
http->send_buffer = NULL; /* clear the pointer */
}
#ifdef USE_NGHTTP2
if(http->header_recvbuf) {
DEBUGF(infof(data, "free header_recvbuf!!\n"));
Curl_add_buffer_free(http->header_recvbuf);
http->header_recvbuf = NULL; /* clear the pointer */
Curl_add_buffer_free(http->trailer_recvbuf);
http->trailer_recvbuf = NULL; /* clear the pointer */
if(http->push_headers) {
/* if they weren't used and then freed before */
for(; http->push_headers_used > 0; --http->push_headers_used) {
free(http->push_headers[http->push_headers_used - 1]);
}
free(http->push_headers);
http->push_headers = NULL;
}
}
if(http->stream_id) {
nghttp2_session_set_stream_user_data(httpc->h2, http->stream_id, 0);
http->stream_id = 0;
}
#endif
Curl_http2_done(conn, premature);
if(HTTPREQ_POST_FORM == data->set.httpreq) {
data->req.bytecount = http->readbytecount + http->writebytecount;
@ -3118,7 +3097,7 @@ CURLcode Curl_http_readwrite_headers(struct Curl_easy *data,
signal the end of the document. */
infof(data, "no chunk, no close, no size. Assume close to "
"signal end\n");
connclose(conn, "HTTP: No end-of-message indicator");
streamclose(conn, "HTTP: No end-of-message indicator");
}
}
@ -3199,7 +3178,7 @@ CURLcode Curl_http_readwrite_headers(struct Curl_easy *data,
*/
if(!k->upload_done) {
infof(data, "HTTP error before end of send, stop sending\n");
connclose(conn, "Stop sending data before everything sent");
streamclose(conn, "Stop sending data before everything sent");
k->upload_done = TRUE;
k->keepon &= ~KEEP_SEND; /* don't send */
if(data->state.expect100header)
@ -3503,7 +3482,7 @@ CURLcode Curl_http_readwrite_headers(struct Curl_easy *data,
/* Negative Content-Length is really odd, and we know it
happens for example when older Apache servers send large
files */
connclose(conn, "negative content-length");
streamclose(conn, "negative content-length");
infof(data, "Negative content-length: %" CURL_FORMAT_CURL_OFF_T
", closing after transfer\n", contentlength);
}
@ -3576,7 +3555,7 @@ CURLcode Curl_http_readwrite_headers(struct Curl_easy *data,
* the connection will close when this request has been
* served.
*/
connclose(conn, "Connection: close used");
streamclose(conn, "Connection: close used");
}
else if(checkprefix("Transfer-Encoding:", k->p)) {
/* One or more encodings. We check for chunked and/or a compression

48
lib/http2.c
View File

@ -185,7 +185,7 @@ const struct Curl_handler Curl_handler_http2 = {
ZERO_NULL, /* readwrite */
PORT_HTTP, /* defport */
CURLPROTO_HTTP, /* protocol */
PROTOPT_NONE /* flags */
PROTOPT_STREAM /* flags */
};
const struct Curl_handler Curl_handler_http2_ssl = {
@ -205,7 +205,7 @@ const struct Curl_handler Curl_handler_http2_ssl = {
ZERO_NULL, /* readwrite */
PORT_HTTP, /* defport */
CURLPROTO_HTTPS, /* protocol */
PROTOPT_SSL /* flags */
PROTOPT_SSL | PROTOPT_STREAM /* flags */
};
/*
@ -489,8 +489,11 @@ static int on_frame_recv(nghttp2_session *session, const nghttp2_frame *frame,
}
stream = data_s->req.protop;
if(!stream)
if(!stream) {
DEBUGF(infof(conn->data, "No proto pointer for stream: %x\n",
stream_id));
return NGHTTP2_ERR_CALLBACK_FAILURE;
}
DEBUGF(infof(data_s, "on_frame_recv() header %x stream %x\n",
frame->hd.type, stream_id));
@ -979,6 +982,43 @@ static int error_callback(nghttp2_session *session,
}
#endif
void Curl_http2_done(struct connectdata *conn, bool premature)
{
struct Curl_easy *data = conn->data;
struct HTTP *http = data->req.protop;
struct http_conn *httpc = &conn->proto.httpc;
if(http->header_recvbuf) {
DEBUGF(infof(data, "free header_recvbuf!!\n"));
Curl_add_buffer_free(http->header_recvbuf);
http->header_recvbuf = NULL; /* clear the pointer */
Curl_add_buffer_free(http->trailer_recvbuf);
http->trailer_recvbuf = NULL; /* clear the pointer */
if(http->push_headers) {
/* if they weren't used and then freed before */
for(; http->push_headers_used > 0; --http->push_headers_used) {
free(http->push_headers[http->push_headers_used - 1]);
}
free(http->push_headers);
http->push_headers = NULL;
}
}
if(premature) {
/* RST_STREAM */
nghttp2_submit_rst_stream(httpc->h2, NGHTTP2_FLAG_NONE, http->stream_id,
NGHTTP2_STREAM_CLOSED);
if(http->stream_id == httpc->pause_stream_id) {
infof(data, "stopped the pause stream!\n");
httpc->pause_stream_id = 0;
}
}
if(http->stream_id) {
nghttp2_session_set_stream_user_data(httpc->h2, http->stream_id, 0);
http->stream_id = 0;
}
}
/*
* Initialize nghttp2 for a Curl connection
*/
@ -1378,6 +1418,8 @@ static ssize_t http2_recv(struct connectdata *conn, int sockindex,
socket is not read. But it seems that usually streams are
notified with its drain property, and socket is read again
quickly. */
DEBUGF(infof(data, "stream %x is paused, pause id: %x\n",
stream->stream_id, httpc->pause_stream_id));
*err = CURLE_AGAIN;
return -1;
}

4
lib/http2.h
View File

@ -7,7 +7,7 @@
* | (__| |_| | _ <| |___
* \___|\___/|_| \_\_____|
*
* Copyright (C) 1998 - 2015, Daniel Stenberg, <daniel@haxx.se>, et al.
* Copyright (C) 1998 - 2016, Daniel Stenberg, <daniel@haxx.se>, et al.
*
* This software is licensed as described in the file COPYING, which
* you should have received as part of this distribution. The terms
@ -51,6 +51,7 @@ CURLcode Curl_http2_switched(struct connectdata *conn,
/* called from Curl_http_setup_conn */
void Curl_http2_setup_conn(struct connectdata *conn);
void Curl_http2_setup_req(struct Curl_easy *data);
void Curl_http2_done(struct connectdata *conn, bool premature);
#else /* USE_NGHTTP2 */
#define Curl_http2_init(x) CURLE_UNSUPPORTED_PROTOCOL
#define Curl_http2_send_request(x) CURLE_UNSUPPORTED_PROTOCOL
@ -61,6 +62,7 @@ void Curl_http2_setup_req(struct Curl_easy *data);
#define Curl_http2_setup_req(x)
#define Curl_http2_init_state(x)
#define Curl_http2_init_userset(x)
#define Curl_http2_done(x,y)
#endif
#endif /* HEADER_CURL_HTTP2_H */

2
lib/http_proxy.c
View File

@ -574,7 +574,7 @@ CURLcode Curl_proxyCONNECT(struct connectdata *conn,
free(data->req.newurl);
data->req.newurl = NULL;
/* failure, close this connection to avoid re-use */
connclose(conn, "proxy CONNECT failure");
streamclose(conn, "proxy CONNECT failure");
Curl_closesocket(conn, conn->sock[sockindex]);
conn->sock[sockindex] = CURL_SOCKET_BAD;
}

47
lib/multi.c
View File

@ -569,10 +569,9 @@ static CURLcode multi_done(struct connectdata **connp,
result = CURLE_ABORTED_BY_CALLBACK;
}
if((!premature &&
conn->send_pipe->size + conn->recv_pipe->size != 0 &&
!data->set.reuse_forbid &&
!conn->bits.close)) {
if(conn->send_pipe->size + conn->recv_pipe->size != 0 &&
!data->set.reuse_forbid &&
!conn->bits.close) {
/* Stop if pipeline is not empty and we do not have to close
connection. */
DEBUGF(infof(data, "Connection still in use, no more multi_done now!\n"));
@ -685,7 +684,7 @@ CURLMcode curl_multi_remove_handle(struct Curl_multi *multi,
/* 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. */
connclose(data->easy_conn, "Removed with partial response");
streamclose(data->easy_conn, "Removed with partial response");
/* Set connection owner so that the DONE function closes it. We can
safely do this here since connection is killed. */
data->easy_conn->data = easy;
@ -1298,7 +1297,9 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
return CURLM_BAD_EASY_HANDLE;
do {
bool disconnect_conn = FALSE;
/* A "stream" here is a logical stream if the protocol can handle that
(HTTP/2), or the full connection for older protocols */
bool stream_error = FALSE;
rc = CURLM_OK;
/* Handle the case when the pipe breaks, i.e., the connection
@ -1376,8 +1377,8 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
/* Force connection closed if the connection has indeed been used */
if(data->mstate > CURLM_STATE_DO) {
connclose(data->easy_conn, "Disconnected with pending data");
disconnect_conn = TRUE;
streamclose(data->easy_conn, "Disconnected with pending data");
stream_error = TRUE;
}
result = CURLE_OPERATION_TIMEDOUT;
(void)multi_done(&data->easy_conn, result, TRUE);
@ -1426,7 +1427,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
/* Add this handle to the send or pend pipeline */
result = Curl_add_handle_to_pipeline(data, data->easy_conn);
if(result)
disconnect_conn = TRUE;
stream_error = TRUE;
else {
if(async)
/* We're now waiting for an asynchronous name lookup */
@ -1518,7 +1519,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
if(result) {
/* failure detected */
disconnect_conn = TRUE;
stream_error = TRUE;
break;
}
}
@ -1558,7 +1559,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
else if(result) {
/* failure detected */
/* Just break, the cleaning up is handled all in one place */
disconnect_conn = TRUE;
stream_error = TRUE;
break;
}
break;
@ -1578,7 +1579,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
/* failure detected */
Curl_posttransfer(data);
multi_done(&data->easy_conn, result, TRUE);
disconnect_conn = TRUE;
stream_error = TRUE;
}
break;
@ -1595,7 +1596,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
/* failure detected */
Curl_posttransfer(data);
multi_done(&data->easy_conn, result, TRUE);
disconnect_conn = TRUE;
stream_error = TRUE;
}
break;
@ -1670,7 +1671,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
if(drc) {
/* a failure here pretty much implies an out of memory */
result = drc;
disconnect_conn = TRUE;
stream_error = TRUE;
}
else
retry = (newurl)?TRUE:FALSE;
@ -1703,7 +1704,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
}
else {
/* Have error handler disconnect conn if we can't retry */
disconnect_conn = TRUE;
stream_error = TRUE;
free(newurl);
}
}
@ -1712,7 +1713,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
Curl_posttransfer(data);
if(data->easy_conn)
multi_done(&data->easy_conn, result, FALSE);
disconnect_conn = TRUE;
stream_error = TRUE;
}
}
break;
@ -1734,7 +1735,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
/* failure detected */
Curl_posttransfer(data);
multi_done(&data->easy_conn, result, FALSE);
disconnect_conn = TRUE;
stream_error = TRUE;
}
break;
@ -1763,7 +1764,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
/* failure detected */
Curl_posttransfer(data);
multi_done(&data->easy_conn, result, FALSE);
disconnect_conn = TRUE;
stream_error = TRUE;
}
break;
@ -1885,10 +1886,10 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
if(!(data->easy_conn->handler->flags & PROTOPT_DUAL) &&
result != CURLE_HTTP2_STREAM)
connclose(data->easy_conn, "Transfer returned error");
streamclose(data->easy_conn, "Transfer returned error");
Curl_posttransfer(data);
multi_done(&data->easy_conn, result, FALSE);
multi_done(&data->easy_conn, result, TRUE);
}
else if(done) {
followtype follow=FOLLOW_NONE;
@ -1944,7 +1945,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
if(!result)
newurl = NULL; /* allocation was handed over Curl_follow() */
else
disconnect_conn = TRUE;
stream_error = TRUE;
}
multistate(data, CURLM_STATE_DONE);
@ -2045,7 +2046,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
Curl_removeHandleFromPipeline(data, data->easy_conn->send_pipe);
Curl_removeHandleFromPipeline(data, data->easy_conn->recv_pipe);
if(disconnect_conn) {
if(stream_error) {
/* Don't attempt to send data over a connection that timed out */
bool dead_connection = result == CURLE_OPERATION_TIMEDOUT;
/* disconnect properly */
@ -2069,7 +2070,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
/* aborted due to progress callback return code must close the
connection */
result = CURLE_ABORTED_BY_CALLBACK;
connclose(data->easy_conn, "Aborted by callback");
streamclose(data->easy_conn, "Aborted by callback");
/* if not yet in DONE state, go there, otherwise COMPLETED */
multistate(data, (data->mstate < CURLM_STATE_DONE)?

11
lib/url.c
View File

@ -2830,6 +2830,17 @@ CURLcode Curl_disconnect(struct connectdata *conn, bool dead_connection)
return CURLE_OK;
}
/*
* If this connection isn't marked to force-close, leave it open if there
* are other users of it
*/
if(!conn->bits.close &&
(conn->send_pipe->size + conn->recv_pipe->size)) {
DEBUGF(infof(data, "Curl_disconnect, usecounter: %d\n",
conn->send_pipe->size + conn->recv_pipe->size));
return CURLE_OK;
}
if(conn->dns_entry != NULL) {
Curl_resolv_unlock(data, conn->dns_entry);
conn->dns_entry = NULL;

1
lib/urldata.h
View File

@ -818,6 +818,7 @@ struct Curl_handler {
#define PROTOPT_CREDSPERREQUEST (1<<7) /* requires login credentials per
request instead of per connection */
#define PROTOPT_ALPN_NPN (1<<8) /* set ALPN and/or NPN for this */
#define PROTOPT_STREAM (1<<9) /* a protocol with individual logical streams */
/* return the count of bytes sent, or -1 on error */
typedef ssize_t (Curl_send)(struct connectdata *conn, /* connection data */

5
tests/data/test161
View File

@ -32,8 +32,8 @@ ftp://%HOSTIP:%FTPPORT/161
# Verify data after the test has been "shot"
<verify>
# This gets QUIT sent because CURLE_PARTIAL_FILE does NOT mark the control
# connection as bad
# This doesn't send QUIT because of known bug:
# "7.8 Premature transfer end but healthy control channel"
<protocol>
USER anonymous
PASS ftp@example.com
@ -42,7 +42,6 @@ EPSV
TYPE I
SIZE 161
RETR 161
QUIT
</protocol>
# CURLE_PARTIAL_FILE = 18
<errorcode>