Skip to content

Commit 4091e9c

Browse files
icingicing
committed
no GOAWAYs on connection failures, sending last received stream id back in GOAWAY, no more flush attempts when session has already been aborted
git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1704262 13f79535-47bb-0310-9956-ffa450edef68
1 parent ff4152d commit 4091e9c

5 files changed

Lines changed: 103 additions & 65 deletions

File tree

‎modules/http2/h2_conn.c‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -288,24 +288,24 @@ apr_status_t h2_session_process(h2_session *session)
288288
|| session->frames_received <= 1)?
289289
APR_BLOCK_READ : APR_NONBLOCK_READ);
290290
switch (status) {
291-
case APR_SUCCESS:
292-
/* successful read, reset our idle timers */
291+
case APR_SUCCESS: /* successful read, reset our idle timers */
293292
have_read = 1;
294293
wait_micros = 0;
295294
break;
296-
case APR_EAGAIN:
295+
case APR_EAGAIN: /* non-blocking read, nothing there */
297296
break;
298-
case APR_EBADF:
297+
case APR_EBADF: /* connection is not there any more */
299298
case APR_EOF:
300299
case APR_ECONNABORTED:
301300
case APR_ECONNRESET:
301+
case APR_TIMEUP: /* blocked read, timed out */
302302
ap_log_cerror( APLOG_MARK, APLOG_DEBUG, status, session->c,
303303
"h2_session(%ld): reading",
304304
session->id);
305305
h2_session_abort(session, status, 0);
306306
break;
307307
default:
308-
ap_log_cerror( APLOG_MARK, APLOG_WARNING, status, session->c,
308+
ap_log_cerror( APLOG_MARK, APLOG_INFO, status, session->c,
309309
APLOGNO(02950)
310310
"h2_session(%ld): error reading, terminating",
311311
session->id);

‎modules/http2/h2_session.c‎

Lines changed: 57 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,9 @@ static int stream_open(h2_session *session, int stream_id)
6464
stream = h2_mplx_open_io(session->mplx, stream_id);
6565
if (stream) {
6666
h2_stream_set_add(session->streams, stream);
67+
if (stream->id > session->max_stream_received) {
68+
session->max_stream_received = stream->id;
69+
}
6770

6871
ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c,
6972
"h2_session: stream(%ld-%d): opened",
@@ -223,10 +226,25 @@ static int on_frame_not_send_cb(nghttp2_session *ngh2,
223226
return 0;
224227
}
225228

226-
static apr_status_t stream_destroy(h2_session *session, h2_stream *stream)
229+
static apr_status_t stream_destroy(h2_session *session,
230+
h2_stream *stream,
231+
uint32_t error_code)
227232
{
228-
ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c,
229-
"h2_stream(%ld-%d): closing", session->id, (int)stream->id);
233+
if (!error_code) {
234+
ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c,
235+
"h2_stream(%ld-%d): handled, closing",
236+
session->id, (int)stream->id);
237+
if (stream->id > session->max_stream_handled) {
238+
session->max_stream_handled = stream->id;
239+
}
240+
}
241+
else {
242+
ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c,
243+
"h2_stream(%ld-%d): closing with err=%d %s",
244+
session->id, (int)stream->id, (int)error_code,
245+
nghttp2_strerror(error_code));
246+
}
247+
230248
h2_stream_set_remove(session->streams, stream);
231249
return h2_mplx_cleanup_stream(session->mplx, stream);
232250
}
@@ -243,7 +261,7 @@ static int on_stream_close_cb(nghttp2_session *ngh2, int32_t stream_id,
243261
}
244262
stream = h2_stream_set_get(session->streams, stream_id);
245263
if (stream) {
246-
stream_destroy(session, stream);
264+
stream_destroy(session, stream, error_code);
247265
}
248266

249267
if (error_code) {
@@ -655,50 +673,43 @@ void h2_session_destroy(h2_session *session)
655673
}
656674
}
657675

658-
apr_status_t h2_session_goaway(h2_session *session, apr_status_t reason)
659-
{
660-
apr_status_t status = APR_SUCCESS;
661-
int rv;
662-
AP_DEBUG_ASSERT(session);
663-
if (session->aborted) {
664-
return APR_EINVAL;
665-
}
666-
667-
rv = 0;
668-
if (reason == APR_SUCCESS) {
669-
rv = nghttp2_submit_shutdown_notice(session->ngh2);
670-
}
671-
else {
672-
int err = 0;
673-
int last_id = nghttp2_session_get_last_proc_stream_id(session->ngh2);
674-
rv = nghttp2_submit_goaway(session->ngh2, last_id,
675-
NGHTTP2_FLAG_NONE, err, NULL, 0);
676-
}
677-
if (rv != 0) {
678-
status = APR_EGENERAL;
679-
ap_log_cerror(APLOG_MARK, APLOG_ERR, status, session->c,
680-
APLOGNO(02930) "session(%ld): submit goaway: %s",
681-
session->id, nghttp2_strerror(rv));
682-
}
683-
return status;
684-
}
685-
686676
static apr_status_t h2_session_abort_int(h2_session *session, int reason)
687677
{
688678
AP_DEBUG_ASSERT(session);
689679
if (!session->aborted) {
690680
session->aborted = 1;
691681
if (session->ngh2) {
692-
if (reason) {
693-
ap_log_cerror(APLOG_MARK, (reason == NGHTTP2_ERR_EOF)?
694-
APLOG_DEBUG : APLOG_INFO, 0, session->c,
682+
683+
if (!reason) {
684+
nghttp2_submit_goaway(session->ngh2, NGHTTP2_FLAG_NONE,
685+
session->max_stream_received,
686+
reason, NULL, 0);
687+
nghttp2_session_send(session->ngh2);
688+
h2_conn_io_flush(&session->io);
689+
}
690+
else {
691+
const char *err = nghttp2_strerror(reason);
692+
693+
ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c,
695694
"session(%ld): aborting session, reason=%d %s",
696-
session->id, reason, nghttp2_strerror(reason));
695+
session->id, reason, err);
696+
697+
if (NGHTTP2_ERR_EOF == reason) {
698+
/* This is our way of indication that the connection is
699+
* gone. No use to send any GOAWAY frames. */
700+
nghttp2_session_terminate_session(session->ngh2, reason);
701+
}
702+
else {
703+
/* The connection might still be there and we shut down
704+
* with GOAWAY and reason information. */
705+
nghttp2_submit_goaway(session->ngh2, NGHTTP2_FLAG_NONE,
706+
session->max_stream_received,
707+
reason, (const uint8_t *)err,
708+
strlen(err));
709+
nghttp2_session_send(session->ngh2);
710+
h2_conn_io_flush(&session->io);
711+
}
697712
}
698-
nghttp2_session_terminate_session(session->ngh2, reason);
699-
nghttp2_submit_goaway(session->ngh2, 0, 0, reason, NULL, 0);
700-
nghttp2_session_send(session->ngh2);
701-
h2_conn_io_flush(&session->io);
702713
}
703714
h2_mplx_abort(session->mplx);
704715
}
@@ -714,10 +725,12 @@ apr_status_t h2_session_abort(h2_session *session, apr_status_t reason, int rv)
714725
case APR_ENOMEM:
715726
rv = NGHTTP2_ERR_NOMEM;
716727
break;
717-
case APR_EOF:
718-
rv = 0;
728+
case APR_SUCCESS: /* all fine, just... */
729+
case APR_EOF: /* client closed its end... */
730+
case APR_TIMEUP: /* got bored waiting... */
731+
rv = 0; /* ...gracefully shut down */
719732
break;
720-
case APR_EBADF:
733+
case APR_EBADF: /* connection unusable, terminate silently */
721734
case APR_ECONNABORTED:
722735
rv = NGHTTP2_ERR_EOF;
723736
break;
@@ -1006,7 +1019,7 @@ apr_status_t h2_session_read(h2_session *session, apr_read_type_e block)
10061019
apr_status_t h2_session_close(h2_session *session)
10071020
{
10081021
AP_DEBUG_ASSERT(session);
1009-
return h2_conn_io_flush(&session->io);
1022+
return session->aborted? APR_SUCCESS : h2_conn_io_flush(&session->io);
10101023
}
10111024

10121025
/* The session wants to send more DATA for the given stream.

‎modules/http2/h2_session.h‎

Lines changed: 40 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -71,44 +71,70 @@ struct h2_session {
7171

7272
struct h2_stream_set *streams; /* streams handled by this session */
7373

74+
int max_stream_received; /* highest stream id created */
75+
int max_stream_handled; /* highest stream id handled successfully */
76+
7477
struct nghttp2_session *ngh2; /* the nghttp2 session (internal use) */
7578
struct h2_workers *workers; /* for executing stream tasks */
7679
};
7780

7881

79-
/* Create a new h2_session for the given connection (mode 'h2').
82+
/**
83+
* Create a new h2_session for the given connection.
8084
* The session will apply the configured parameter.
85+
* @param c the connection to work on
86+
* @param cfg the module config to apply
87+
* @param workers the worker pool to use
88+
* @return the created session
8189
*/
8290
h2_session *h2_session_create(conn_rec *c, struct h2_config *cfg,
8391
struct h2_workers *workers);
8492

85-
/* Create a new h2_session for the given request (mode 'h2c').
93+
/**
94+
* Create a new h2_session for the given request.
8695
* The session will apply the configured parameter.
96+
* @param r the request that was upgraded
97+
* @param cfg the module config to apply
98+
* @param workers the worker pool to use
99+
* @return the created session
87100
*/
88101
h2_session *h2_session_rcreate(request_rec *r, struct h2_config *cfg,
89102
struct h2_workers *workers);
90103

91-
/* Destroy the session and all object it still contains. This will not
92-
* destroy h2_task instances that not finished yet. */
104+
/**
105+
* Destroy the session and all objects it still contains. This will not
106+
* destroy h2_task instances that have not finished yet.
107+
* @param session the session to destroy
108+
*/
93109
void h2_session_destroy(h2_session *session);
94110

95-
/* Called once at start of session. Performs initial client thingies. */
111+
/**
112+
* Called once at start of session.
113+
* Sets up the session and sends the initial SETTINGS frame.
114+
* @param session the session to start
115+
* @param rv error codes in libnghttp2 lingo are returned here
116+
* @return APR_SUCCESS if all went well
117+
*/
96118
apr_status_t h2_session_start(h2_session *session, int *rv);
97119

98-
/* Return != 0 iff session is finished and connection can be closed.
120+
/**
121+
* Determine if session is finished.
122+
* @return != 0 iff session is finished and connection can be closed.
99123
*/
100124
int h2_session_is_done(h2_session *session);
101125

102-
/* Called when the session will shutdown after all open streams
103-
* are handled. New streams will no longer be accepted.
104-
* Call with reason APR_SUCCESS to initiate a graceful shutdown. */
105-
apr_status_t h2_session_goaway(h2_session *session, apr_status_t reason);
106-
107-
/* Called when an error occured and the session needs to shut down.
108-
* Status indicates the reason of the error. */
126+
/**
127+
* Called when an error occured and the session needs to shut down.
128+
* @param session the session to shut down
129+
* @param reason the apache status that caused the shutdown
130+
* @param rv the nghttp2 reason for shutdown, set to 0 if you have none.
131+
*
132+
*/
109133
apr_status_t h2_session_abort(h2_session *session, apr_status_t reason, int rv);
110134

111-
/* Called before a session gets destroyed, might flush output etc. */
135+
/**
136+
* Called before a session gets destroyed, might flush output etc.
137+
*/
112138
apr_status_t h2_session_close(h2_session *session);
113139

114140
/* Read more data from the client connection. Used normally with blocking

‎modules/http2/h2_stream.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ h2_stream *h2_stream_create(int id, apr_pool_t *pool, struct h2_mplx *m)
6161
return stream;
6262
}
6363

64-
void h2_stream_cleanup(h2_stream *stream)
64+
static void h2_stream_cleanup(h2_stream *stream)
6565
{
6666
if (stream->request) {
6767
h2_request_destroy(stream->request);

‎modules/http2/h2_stream.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,6 @@ struct h2_stream {
7272
h2_stream *h2_stream_create(int id, apr_pool_t *pool, struct h2_mplx *m);
7373

7474
apr_status_t h2_stream_destroy(h2_stream *stream);
75-
void h2_stream_cleanup(h2_stream *stream);
7675

7776
apr_pool_t *h2_stream_detach_pool(h2_stream *stream);
7877
void h2_stream_attach_pool(h2_stream *stream, apr_pool_t *pool);

0 commit comments

Comments
 (0)