From c32d0e9adfe1e3490f74b0ffb79225fa2a908782 Mon Sep 17 00:00:00 2001 From: Joe Wilm Date: Thu, 6 Oct 2016 17:23:18 -0700 Subject: [PATCH] fix(http): stackoverflow in Conn::ready I've had a couple of instances during stress testing now where Conn::ready would overflow its stack due to recursing on itself. This moves subsequent calls to ready() into a loop outside the function. --- src/client/mod.rs | 15 +++++++++++---- src/http/conn.rs | 39 +++++++++++++++++++++++++++++++-------- src/http/mod.rs | 2 +- src/server/mod.rs | 22 +++++++++++++++------- 4 files changed, 58 insertions(+), 20 deletions(-) diff --git a/src/client/mod.rs b/src/client/mod.rs index 1a90a246f0..1f4a7aa385 100644 --- a/src/client/mod.rs +++ b/src/client/mod.rs @@ -14,7 +14,7 @@ use std::time::Duration; use rotor::{self, Scope, EventSet, PollOpt}; use header::Host; -use http::{self, Next, RequestHead}; +use http::{self, Next, RequestHead, ReadyResult}; use net::Transport; use uri::RequestUri; use {Url}; @@ -467,9 +467,16 @@ where C: Connect, fn ready(self, events: EventSet, scope: &mut Scope) -> rotor::Response { match self { ClientFsm::Socket(conn) => { - let res = conn.ready(events, scope); - let now = scope.now(); - conn_response!(scope, res, now) + let mut conn = Some(conn); + loop { + match conn.take().unwrap().ready(events, scope) { + ReadyResult::Done(res) => { + let now = scope.now(); + return conn_response!(scope, res, now); + }, + ReadyResult::Continue(c) => conn = Some(c), + } + } }, ClientFsm::Connecting(mut seed) => { if events.is_error() || events.is_hup() { diff --git a/src/http/conn.rs b/src/http/conn.rs index 9ea90b85c4..b30738c625 100644 --- a/src/http/conn.rs +++ b/src/http/conn.rs @@ -496,6 +496,11 @@ impl> ConnInner { } +pub enum ReadyResult { + Continue(C), + Done(Option<(C, Option)>) +} + impl> Conn { pub fn new(key: K, transport: T, next: Next, notify: rotor::Notifier) -> Conn { Conn(Box::new(ConnInner { @@ -516,8 +521,13 @@ impl> Conn { self } - pub fn ready(mut self, events: EventSet, scope: &mut Scope) -> Option<(Self, Option)> - where F: MessageHandlerFactory { + pub fn ready( + mut self, + events: EventSet, + scope: &mut Scope + ) -> ReadyResult + where F: MessageHandlerFactory + { trace!("Conn::ready events='{:?}', blocked={:?}", events, self.0.transport.blocked()); if events.is_error() { @@ -579,24 +589,24 @@ impl> Conn { trace!("removing transport"); let _ = scope.deregister(&self.0.transport); self.on_remove(); - return None; + return ReadyResult::Done(None); }, }; if events.is_readable() && self.0.can_read_more(was_init) { - return self.ready(events, scope); + return ReadyResult::Continue(self); } trace!("scope.reregister({:?})", events); match scope.reregister(&self.0.transport, events, PollOpt::level()) { Ok(..) => { let timeout = self.0.state.timeout(); - Some((self, timeout)) + ReadyResult::Done(Some((self, timeout))) }, Err(e) => { trace!("error reregistering: {:?}", e); self.0.on_error(e.into(), &**scope); - None + ReadyResult::Done(None) } } } @@ -607,14 +617,27 @@ impl> Conn { trace!("woke up with {:?}", next); self.0.state.update(next, &**scope); } - self.ready(EventSet::readable() | EventSet::writable(), scope) + + let mut conn = Some(self); + loop { + match conn.take().unwrap().ready(EventSet::readable() | EventSet::writable(), scope) { + ReadyResult::Done(val) => return val, + ReadyResult::Continue(c) => conn = Some(c), + } + } } pub fn timeout(mut self, scope: &mut Scope) -> Option<(Self, Option)> where F: MessageHandlerFactory { //TODO: check if this was a spurious timeout? self.0.on_error(::Error::Timeout, &**scope); - self.ready(EventSet::none(), scope) + let mut conn = Some(self); + loop { + match conn.take().unwrap().ready(EventSet::none(), scope) { + ReadyResult::Done(val) => return val, + ReadyResult::Continue(c) => conn = Some(c), + } + } } fn on_remove(self) { diff --git a/src/http/mod.rs b/src/http/mod.rs index 554833df06..1a3422f92f 100644 --- a/src/http/mod.rs +++ b/src/http/mod.rs @@ -17,7 +17,7 @@ use version::HttpVersion::{Http10, Http11}; #[cfg(feature = "serde-serialization")] use serde::{Deserialize, Deserializer, Serialize, Serializer}; -pub use self::conn::{Conn, MessageHandler, MessageHandlerFactory, Seed, Key}; +pub use self::conn::{Conn, MessageHandler, MessageHandlerFactory, Seed, Key, ReadyResult}; mod buffer; pub mod channel; diff --git a/src/server/mod.rs b/src/server/mod.rs index fbb537705c..88e778c15e 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -14,7 +14,7 @@ use rotor::{self, Scope}; pub use self::request::Request; pub use self::response::Response; -use http::{self, Next}; +use http::{self, Next, ReadyResult}; pub use net::{Accept, HttpListener, HttpsListener}; use net::{SslServer, Transport}; @@ -248,13 +248,21 @@ where A: Accept, } }, ServerFsm::Conn(conn) => { - match conn.ready(events, scope) { - Some((conn, None)) => rotor::Response::ok(ServerFsm::Conn(conn)), - Some((conn, Some(dur))) => { - rotor::Response::ok(ServerFsm::Conn(conn)) - .deadline(scope.now() + dur) + let mut conn = Some(conn); + loop { + match conn.take().unwrap().ready(events, scope) { + ReadyResult::Continue(c) => conn = Some(c), + ReadyResult::Done(res) => { + return match res { + Some((conn, None)) => rotor::Response::ok(ServerFsm::Conn(conn)), + Some((conn, Some(dur))) => { + rotor::Response::ok(ServerFsm::Conn(conn)) + .deadline(scope.now() + dur) + } + None => rotor::Response::done() + }; + } } - None => rotor::Response::done() } } }