From 5770c6bd043e7d0dbd5ca839bb1951a3e3c87f37 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 21 Jan 2015 23:30:48 +0900 Subject: [PATCH] nghttpx: Return 503 on hard disconnect in HTTP/2 backend --- integration-tests/nghttpx_test.go | 57 +++++++++++++++++++++++++++++++ src/shrpx_http2_session.cc | 6 +--- src/shrpx_http2_upstream.cc | 12 +++++-- src/shrpx_http2_upstream.h | 2 +- src/shrpx_https_upstream.cc | 9 ++++- src/shrpx_https_upstream.h | 2 +- src/shrpx_spdy_upstream.cc | 12 +++++-- src/shrpx_spdy_upstream.h | 2 +- src/shrpx_upstream.h | 5 +-- 9 files changed, 92 insertions(+), 15 deletions(-) diff --git a/integration-tests/nghttpx_test.go b/integration-tests/nghttpx_test.go index 48b1b308..b2bc317d 100644 --- a/integration-tests/nghttpx_test.go +++ b/integration-tests/nghttpx_test.go @@ -95,6 +95,25 @@ func TestH1H1ConnectFailure(t *testing.T) { } } +func TestH1H2ConnectFailure(t *testing.T) { + st := newServerTester([]string{"--http2-bridge"}, t, noopHandler) + defer st.Close() + + // simulate backend connect attempt failure + st.ts.Close() + + res, err := st.http1(requestParam{ + name: "TestH1H2ConnectFailure", + }) + if err != nil { + t.Fatalf("Error st.http1() = %v", err) + } + want := 503 + if got := res.status; got != want { + t.Errorf("status: %v; want %v", got, want) + } +} + func TestH1H2NoHost(t *testing.T) { st := newServerTester([]string{"--http2-bridge"}, t, func(w http.ResponseWriter, r *http.Request) { t.Errorf("server should not forward bad request") @@ -409,6 +428,25 @@ func TestH2H2InvalidResponseCL(t *testing.T) { } } +func TestH2H2ConnectFailure(t *testing.T) { + st := newServerTester([]string{"--http2-bridge"}, t, noopHandler) + defer st.Close() + + // simulate backend connect attempt failure + st.ts.Close() + + res, err := st.http2(requestParam{ + name: "TestH2H2ConnectFailure", + }) + if err != nil { + t.Fatalf("Error st.http2() = %v", err) + } + want := 503 + if got := res.status; got != want { + t.Errorf("status: %v; want %v", got, want) + } +} + func TestS3H1PlainGET(t *testing.T) { st := newServerTesterTLS([]string{"--npn-list=spdy/3.1"}, t, noopHandler) defer st.Close() @@ -492,3 +530,22 @@ func TestS3H1InvalidRequestCL(t *testing.T) { t.Errorf("status: %v; want %v", got, want) } } + +func TestS3H2ConnectFailure(t *testing.T) { + st := newServerTesterTLS([]string{"--npn-list=spdy/3.1", "--http2-bridge"}, t, noopHandler) + defer st.Close() + + // simulate backend connect attempt failure + st.ts.Close() + + res, err := st.spdy(requestParam{ + name: "TestS3H2ConnectFailure", + }) + if err != nil { + t.Fatalf("Error st.spdy() = %v", err) + } + want := 503 + if got := res.status; got != want { + t.Errorf("status: %v; want %v", got, want) + } +} diff --git a/src/shrpx_http2_session.cc b/src/shrpx_http2_session.cc index 2c7f108f..14dfdebf 100644 --- a/src/shrpx_http2_session.cc +++ b/src/shrpx_http2_session.cc @@ -239,11 +239,7 @@ int Http2Session::disconnect(bool hard) { handlers.insert(dc->get_client_handler()); } for (auto h : handlers) { - if (hard) { - delete h; - continue; - } - if (h->get_upstream()->on_downstream_reset() != 0) { + if (h->get_upstream()->on_downstream_reset(hard) != 0) { delete h; } } diff --git a/src/shrpx_http2_upstream.cc b/src/shrpx_http2_upstream.cc index 8c3ae005..2a550b01 100644 --- a/src/shrpx_http2_upstream.cc +++ b/src/shrpx_http2_upstream.cc @@ -1345,7 +1345,7 @@ void Http2Upstream::on_handler_delete() { } } -int Http2Upstream::on_downstream_reset() { +int Http2Upstream::on_downstream_reset(bool no_retry) { int rv; for (auto &ent : downstream_queue_.get_active_downstreams()) { @@ -1358,9 +1358,17 @@ int Http2Upstream::on_downstream_reset() { continue; } + downstream->pop_downstream_connection(); + + if (no_retry) { + if (on_downstream_abort_request(downstream, 503) != 0) { + return -1; + } + continue; + } + // downstream connection is clean; we can retry with new // downstream connection. - downstream->pop_downstream_connection(); rv = downstream->attach_downstream_connection( handler_->get_downstream_connection()); diff --git a/src/shrpx_http2_upstream.h b/src/shrpx_http2_upstream.h index c0d21f46..ee562c1b 100644 --- a/src/shrpx_http2_upstream.h +++ b/src/shrpx_http2_upstream.h @@ -80,7 +80,7 @@ public: virtual int on_downstream_body_complete(Downstream *downstream); virtual void on_handler_delete(); - virtual int on_downstream_reset(); + virtual int on_downstream_reset(bool no_retry); virtual MemchunkPool *get_mcpool(); diff --git a/src/shrpx_https_upstream.cc b/src/shrpx_https_upstream.cc index d7564c06..9ef2a9e3 100644 --- a/src/shrpx_https_upstream.cc +++ b/src/shrpx_https_upstream.cc @@ -815,7 +815,7 @@ void HttpsUpstream::on_handler_delete() { } } -int HttpsUpstream::on_downstream_reset() { +int HttpsUpstream::on_downstream_reset(bool no_retry) { int rv; if ((downstream_->get_request_state() != Downstream::HEADER_COMPLETE && @@ -825,6 +825,13 @@ int HttpsUpstream::on_downstream_reset() { return -1; } + if (no_retry) { + if (on_downstream_abort_request(downstream_.get(), 503) != 0) { + return -1; + } + return 0; + } + downstream_->pop_downstream_connection(); rv = downstream_->attach_downstream_connection( diff --git a/src/shrpx_https_upstream.h b/src/shrpx_https_upstream.h index 77f00522..820d462c 100644 --- a/src/shrpx_https_upstream.h +++ b/src/shrpx_https_upstream.h @@ -74,7 +74,7 @@ public: virtual int on_downstream_body_complete(Downstream *downstream); virtual void on_handler_delete(); - virtual int on_downstream_reset(); + virtual int on_downstream_reset(bool no_retry); virtual MemchunkPool *get_mcpool(); diff --git a/src/shrpx_spdy_upstream.cc b/src/shrpx_spdy_upstream.cc index 3ec91e94..b19e46a6 100644 --- a/src/shrpx_spdy_upstream.cc +++ b/src/shrpx_spdy_upstream.cc @@ -1028,7 +1028,7 @@ void SpdyUpstream::on_handler_delete() { } } -int SpdyUpstream::on_downstream_reset() { +int SpdyUpstream::on_downstream_reset(bool no_retry) { int rv; for (auto &ent : downstream_queue_.get_active_downstreams()) { @@ -1041,9 +1041,17 @@ int SpdyUpstream::on_downstream_reset() { continue; } + downstream->pop_downstream_connection(); + + if (no_retry) { + if (on_downstream_abort_request(downstream, 503) != 0) { + return -1; + } + return 0; + } + // downstream connection is clean; we can retry with new // downstream connection. - downstream->pop_downstream_connection(); rv = downstream->attach_downstream_connection( handler_->get_downstream_connection()); diff --git a/src/shrpx_spdy_upstream.h b/src/shrpx_spdy_upstream.h index 639af319..9d83fc67 100644 --- a/src/shrpx_spdy_upstream.h +++ b/src/shrpx_spdy_upstream.h @@ -75,7 +75,7 @@ public: virtual int on_downstream_body_complete(Downstream *downstream); virtual void on_handler_delete(); - virtual int on_downstream_reset(); + virtual int on_downstream_reset(bool no_retry); virtual MemchunkPool *get_mcpool(); diff --git a/src/shrpx_upstream.h b/src/shrpx_upstream.h index 3358510e..ad8e7420 100644 --- a/src/shrpx_upstream.h +++ b/src/shrpx_upstream.h @@ -58,8 +58,9 @@ public: virtual void on_handler_delete() = 0; // Called when downstream connection is reset. Currently this is - // only used by Http2Session. - virtual int on_downstream_reset() = 0; + // only used by Http2Session. If |no_retry| is true, another + // connection attempt using new DownstreamConnection is not allowed. + virtual int on_downstream_reset(bool no_retry) = 0; virtual void pause_read(IOCtrlReason reason) = 0; virtual int resume_read(IOCtrlReason reason, Downstream *downstream,