From 81653c1d1b8f1bf01ddb0f6a9c82148fc65667fd Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 29 Aug 2013 22:58:05 +0900 Subject: [PATCH] Add int return value to nghttp2_on_stream_close_callback --- lib/includes/nghttp2/nghttp2.h | 7 +- lib/nghttp2_session.c | 122 +++++++++++++++++++++++++-------- src/HttpServer.cc | 3 +- src/nghttp.cc | 3 +- src/shrpx_http2_upstream.cc | 3 +- src/shrpx_spdy_session.cc | 5 +- tests/nghttp2_session_test.c | 7 +- 7 files changed, 112 insertions(+), 38 deletions(-) diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h index 0199796e..c5a5298e 100644 --- a/lib/includes/nghttp2/nghttp2.h +++ b/lib/includes/nghttp2/nghttp2.h @@ -911,8 +911,13 @@ typedef int (*nghttp2_on_data_send_callback) * |error_code|. The stream_user_data, which was specified in * `nghttp2_submit_request()` or `nghttp2_submit_headers()`, is * still available in this function. + * + * The implementation of this function must return 0 if it + * succeeds. If nonzero is returned, it is treated as fatal error and + * `nghttp2_session_recv()` and `nghttp2_session_send()` functions + * immediately return :enum:`NGHTTP2_ERR_CALLBACK_FAILURE`. */ -typedef void (*nghttp2_on_stream_close_callback) +typedef int (*nghttp2_on_stream_close_callback) (nghttp2_session *session, int32_t stream_id, nghttp2_error_code error_code, void *user_data); diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index d9e9233a..d1e914ba 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -524,6 +524,18 @@ nghttp2_stream* nghttp2_session_open_stream(nghttp2_session *session, return stream; } +/* + * Closes stream with stream ID |stream_id|. The |error_code| + * indicates the reason of the closure. + * + * This function returns 0 if it succeeds, or one of the following + * negative error codes: + * + * NGHTTP2_ERR_INVALID_ARGUMENT + * The stream is not found. + * NGHTTP2_ERR_CALLBACK_FAILURE + * The callback function failed. + */ int nghttp2_session_close_stream(nghttp2_session *session, int32_t stream_id, nghttp2_error_code error_code) { @@ -534,9 +546,12 @@ int nghttp2_session_close_stream(nghttp2_session *session, int32_t stream_id, /* TODO Should on_stream_close_callback be called against NGHTTP2_STREAM_RESERVED? It is actually not opened yet. */ session->callbacks.on_stream_close_callback) { - session->callbacks.on_stream_close_callback(session, stream_id, - error_code, - session->user_data); + if(session->callbacks.on_stream_close_callback + (session, stream_id, + error_code, + session->user_data) != 0) { + return NGHTTP2_ERR_CALLBACK_FAILURE; + } } if(stream->state != NGHTTP2_STREAM_RESERVED) { if(nghttp2_session_is_my_stream_id(session, stream_id)) { @@ -554,6 +569,19 @@ int nghttp2_session_close_stream(nghttp2_session *session, int32_t stream_id, } } +/* + * Closes stream with stream ID |stream_id| if both transmission and + * reception of the stream were disallowed. The |error_code| indicates + * the reason of the closure. + * + * This function returns 0 if it succeeds, or one of the following + * negative error codes: + * + * NGHTTP2_ERR_INVALID_ARGUMENT + * The stream is not found. + * NGHTTP2_ERR_CALLBACK_FAILURE + * The callback function failed. + */ int nghttp2_session_close_stream_if_shut_rdwr(nghttp2_session *session, nghttp2_stream *stream) { @@ -1256,6 +1284,7 @@ static int nghttp2_session_after_frame_sent(nghttp2_session *session) { nghttp2_outbound_item *item = session->aob.item; if(item->frame_cat == NGHTTP2_CAT_CTRL) { + int r; nghttp2_frame *frame; frame = nghttp2_outbound_item_get_ctrl_frame(session->aob.item); if(session->callbacks.on_frame_send_callback) { @@ -1276,21 +1305,23 @@ static int nghttp2_session_after_frame_sent(nghttp2_session *session) if(frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_WR); } - nghttp2_session_close_stream_if_shut_rdwr(session, stream); + r = nghttp2_session_close_stream_if_shut_rdwr(session, stream); + if(r != 0 && nghttp2_is_fatal(r)) { + return r; + } + r = 0; /* We assume aux_data is a pointer to nghttp2_headers_aux_data */ aux_data = (nghttp2_headers_aux_data*)item->aux_data; if(aux_data && aux_data->data_prd) { - int r; /* nghttp2_submit_data() makes a copy of aux_data->data_prd */ r = nghttp2_submit_data(session, NGHTTP2_FLAG_END_STREAM, frame->hd.stream_id, aux_data->data_prd); - if(r != 0) { - if(nghttp2_is_fatal(r)) { - return r; - } - /* If r is not fatal, the only possible error is closed - stream, so we have nothing to do here. */ + if(r != 0 &&nghttp2_is_fatal(r)) { + return r; } + /* If r is not fatal, the only possible error is closed + stream, so we have nothing to do here. */ + r = 0; } break; } @@ -1300,27 +1331,33 @@ static int nghttp2_session_after_frame_sent(nghttp2_session *session) if(frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_WR); } - nghttp2_session_close_stream_if_shut_rdwr(session, stream); + r = nghttp2_session_close_stream_if_shut_rdwr(session, stream); + if(r != 0 && nghttp2_is_fatal(r)) { + return r; + } + r = 0; /* We assume aux_data is a pointer to nghttp2_headers_aux_data */ aux_data = (nghttp2_headers_aux_data*)item->aux_data; if(aux_data && aux_data->data_prd) { - int r; r = nghttp2_submit_data(session, NGHTTP2_FLAG_END_STREAM, frame->hd.stream_id, aux_data->data_prd); - if(r != 0) { - if(nghttp2_is_fatal(r)) { - return r; - } - /* If r is not fatal, the only possible error is closed - stream, so we have nothing to do here. */ + if(r != 0 && nghttp2_is_fatal(r)) { + return r; } + /* If r is not fatal, the only possible error is closed + stream, so we have nothing to do here. */ + r = 0; } break; case NGHTTP2_HCAT_HEADERS: if(frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_WR); } - nghttp2_session_close_stream_if_shut_rdwr(session, stream); + r = nghttp2_session_close_stream_if_shut_rdwr(session, stream); + if(r != 0 && nghttp2_is_fatal(r)) { + return r; + } + r = 0; break; } } @@ -1330,8 +1367,12 @@ static int nghttp2_session_after_frame_sent(nghttp2_session *session) /* nothing to do */ break; case NGHTTP2_RST_STREAM: - nghttp2_session_close_stream(session, frame->hd.stream_id, - frame->rst_stream.error_code); + r = nghttp2_session_close_stream(session, frame->hd.stream_id, + frame->rst_stream.error_code); + if(r != 0 && nghttp2_is_fatal(r)) { + return r; + } + r = 0; break; case NGHTTP2_SETTINGS: /* nothing to do */ @@ -1370,7 +1411,11 @@ static int nghttp2_session_after_frame_sent(nghttp2_session *session) nghttp2_session_get_stream(session, data_frame->hd.stream_id); if(stream) { nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_WR); - nghttp2_session_close_stream_if_shut_rdwr(session, stream); + r = nghttp2_session_close_stream_if_shut_rdwr(session, stream); + if(r != 0 && nghttp2_is_fatal(r)) { + return r; + } + r = 0; } } /* If session is closed or RST_STREAM was queued, we won't send @@ -1767,7 +1812,11 @@ int nghttp2_session_on_response_headers_received(nghttp2_session *session, /* This is the last frame of this stream, so disallow further receptions. */ nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD); - nghttp2_session_close_stream_if_shut_rdwr(session, stream); + rv = nghttp2_session_close_stream_if_shut_rdwr(session, stream); + if(rv != 0 && nghttp2_is_fatal(rv)) { + return rv; + } + rv = 0; } } else { /* half closed (remote): from the spec: @@ -1836,7 +1885,11 @@ int nghttp2_session_on_headers_received(nghttp2_session *session, } if(frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD); - nghttp2_session_close_stream_if_shut_rdwr(session, stream); + r = nghttp2_session_close_stream_if_shut_rdwr(session, stream); + if(r != 0 && nghttp2_is_fatal(r)) { + return r; + } + r = 0; } } else if(stream->state == NGHTTP2_STREAM_CLOSING) { /* This is race condition. NGHTTP2_STREAM_CLOSING indicates @@ -1858,7 +1911,11 @@ int nghttp2_session_on_headers_received(nghttp2_session *session, if(frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { nghttp2_session_call_on_request_recv(session, frame->hd.stream_id); nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD); - nghttp2_session_close_stream_if_shut_rdwr(session, stream); + r = nghttp2_session_close_stream_if_shut_rdwr(session, stream); + if(r != 0 && nghttp2_is_fatal(r)) { + return r; + } + r = 0; } } } @@ -1920,8 +1977,11 @@ int nghttp2_session_on_rst_stream_received(nghttp2_session *session, if(rv != 0) { return rv; } - nghttp2_session_close_stream(session, frame->hd.stream_id, - frame->rst_stream.error_code); + rv = nghttp2_session_close_stream(session, frame->hd.stream_id, + frame->rst_stream.error_code); + if(rv != 0 && nghttp2_is_fatal(rv)) { + return rv; + } return 0; } @@ -2663,7 +2723,11 @@ int nghttp2_session_on_data_received(nghttp2_session *session, if(valid) { if(flags & NGHTTP2_FLAG_END_STREAM) { nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_RD); - nghttp2_session_close_stream_if_shut_rdwr(session, stream); + r = nghttp2_session_close_stream_if_shut_rdwr(session, stream); + if(r != 0 && nghttp2_is_fatal(r)) { + return r; + } + r = 0; } } } else { diff --git a/src/HttpServer.cc b/src/HttpServer.cc index 1338c3c4..5618261f 100644 --- a/src/HttpServer.cc +++ b/src/HttpServer.cc @@ -792,7 +792,7 @@ int hd_on_data_send_callback } // namespace namespace { -void on_stream_close_callback +int on_stream_close_callback (nghttp2_session *session, int32_t stream_id, nghttp2_error_code error_code, void *user_data) { @@ -804,6 +804,7 @@ void on_stream_close_callback printf(" stream_id=%d closed\n", stream_id); fflush(stdout); } + return 0; } } // namespace diff --git a/src/nghttp.cc b/src/nghttp.cc index 42b86e01..5ca9342d 100644 --- a/src/nghttp.cc +++ b/src/nghttp.cc @@ -1046,7 +1046,7 @@ int on_frame_recv_callback2 return 0; } -void on_stream_close_callback +int on_stream_close_callback (nghttp2_session *session, int32_t stream_id, nghttp2_error_code error_code, void *user_data) { @@ -1060,6 +1060,7 @@ void on_stream_close_callback nghttp2_submit_goaway(session, NGHTTP2_NO_ERROR, NULL, 0); } } + return 0; } void print_stats(const HttpClient& client) diff --git a/src/shrpx_http2_upstream.cc b/src/shrpx_http2_upstream.cc index 95a39a42..ca236723 100644 --- a/src/shrpx_http2_upstream.cc +++ b/src/shrpx_http2_upstream.cc @@ -93,7 +93,7 @@ ssize_t recv_callback(nghttp2_session *session, } // namespace namespace { -void on_stream_close_callback +int on_stream_close_callback (nghttp2_session *session, int32_t stream_id, nghttp2_error_code error_code, void *user_data) { @@ -135,6 +135,7 @@ void on_stream_close_callback } } } + return 0; } } // namespace diff --git a/src/shrpx_spdy_session.cc b/src/shrpx_spdy_session.cc index d2e3e92c..a32f4ab8 100644 --- a/src/shrpx_spdy_session.cc +++ b/src/shrpx_spdy_session.cc @@ -681,7 +681,7 @@ ssize_t recv_callback(nghttp2_session *session, } // namespace namespace { -void on_stream_close_callback +int on_stream_close_callback (nghttp2_session *session, int32_t stream_id, nghttp2_error_code error_code, void *user_data) { @@ -696,7 +696,7 @@ void on_stream_close_callback if(sd == 0) { // We might get this close callback when pushed streams are // closed. - return; + return 0; } auto dconn = sd->dconn; if(dconn) { @@ -718,6 +718,7 @@ void on_stream_close_callback } // The life time of StreamData ends here spdy->remove_stream_data(sd); + return 0; } } // namespace diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index aa4dc33c..b322e34f 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -263,14 +263,15 @@ static ssize_t defer_data_source_read_callback return NGHTTP2_ERR_DEFERRED; } -static void stream_close_callback(nghttp2_session *session, int32_t stream_id, - nghttp2_error_code error_code, - void *user_data) +static int stream_close_callback(nghttp2_session *session, int32_t stream_id, + nghttp2_error_code error_code, + void *user_data) { my_user_data* my_data = (my_user_data*)user_data; void *stream_data = nghttp2_session_get_stream_user_data(session, stream_id); ++my_data->stream_close_cb_called; CU_ASSERT(stream_data != NULL); + return 0; } static nghttp2_settings_entry* dup_iv(const nghttp2_settings_entry *iv,