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.
This commit is contained in:
		| @@ -14,7 +14,7 @@ use std::time::Duration; | |||||||
| use rotor::{self, Scope, EventSet, PollOpt}; | use rotor::{self, Scope, EventSet, PollOpt}; | ||||||
|  |  | ||||||
| use header::Host; | use header::Host; | ||||||
| use http::{self, Next, RequestHead}; | use http::{self, Next, RequestHead, ReadyResult}; | ||||||
| use net::Transport; | use net::Transport; | ||||||
| use uri::RequestUri; | use uri::RequestUri; | ||||||
| use {Url}; | use {Url}; | ||||||
| @@ -467,9 +467,16 @@ where C: Connect, | |||||||
|     fn ready(self, events: EventSet, scope: &mut Scope<Self::Context>) -> rotor::Response<Self, Self::Seed> { |     fn ready(self, events: EventSet, scope: &mut Scope<Self::Context>) -> rotor::Response<Self, Self::Seed> { | ||||||
|         match self { |         match self { | ||||||
|             ClientFsm::Socket(conn) => { |             ClientFsm::Socket(conn) => { | ||||||
|                 let res = conn.ready(events, scope); |                 let mut conn = Some(conn); | ||||||
|                 let now = scope.now(); |                 loop { | ||||||
|                 conn_response!(scope, res, now) |                     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) => { |             ClientFsm::Connecting(mut seed) => { | ||||||
|                 if events.is_error() || events.is_hup() { |                 if events.is_error() || events.is_hup() { | ||||||
|   | |||||||
| @@ -496,6 +496,11 @@ impl<K: Key, T: Transport, H: MessageHandler<T>> ConnInner<K, T, H> { | |||||||
|  |  | ||||||
| } | } | ||||||
|  |  | ||||||
|  | pub enum ReadyResult<C> { | ||||||
|  |     Continue(C), | ||||||
|  |     Done(Option<(C, Option<Duration>)>) | ||||||
|  | } | ||||||
|  |  | ||||||
| impl<K: Key, T: Transport, H: MessageHandler<T>> Conn<K, T, H> { | impl<K: Key, T: Transport, H: MessageHandler<T>> Conn<K, T, H> { | ||||||
|     pub fn new(key: K, transport: T, next: Next, notify: rotor::Notifier) -> Conn<K, T, H> { |     pub fn new(key: K, transport: T, next: Next, notify: rotor::Notifier) -> Conn<K, T, H> { | ||||||
|         Conn(Box::new(ConnInner { |         Conn(Box::new(ConnInner { | ||||||
| @@ -516,8 +521,13 @@ impl<K: Key, T: Transport, H: MessageHandler<T>> Conn<K, T, H> { | |||||||
|         self |         self | ||||||
|     } |     } | ||||||
|  |  | ||||||
|     pub fn ready<F>(mut self, events: EventSet, scope: &mut Scope<F>) -> Option<(Self, Option<Duration>)> |     pub fn ready<F>( | ||||||
|     where F: MessageHandlerFactory<K, T, Output=H> { |         mut self, | ||||||
|  |         events: EventSet, | ||||||
|  |         scope: &mut Scope<F> | ||||||
|  |     ) -> ReadyResult<Self> | ||||||
|  |         where F: MessageHandlerFactory<K, T, Output=H> | ||||||
|  |     { | ||||||
|         trace!("Conn::ready events='{:?}', blocked={:?}", events, self.0.transport.blocked()); |         trace!("Conn::ready events='{:?}', blocked={:?}", events, self.0.transport.blocked()); | ||||||
|  |  | ||||||
|         if events.is_error() { |         if events.is_error() { | ||||||
| @@ -579,24 +589,24 @@ impl<K: Key, T: Transport, H: MessageHandler<T>> Conn<K, T, H> { | |||||||
|                 trace!("removing transport"); |                 trace!("removing transport"); | ||||||
|                 let _ = scope.deregister(&self.0.transport); |                 let _ = scope.deregister(&self.0.transport); | ||||||
|                 self.on_remove(); |                 self.on_remove(); | ||||||
|                 return None; |                 return ReadyResult::Done(None); | ||||||
|             }, |             }, | ||||||
|         }; |         }; | ||||||
|  |  | ||||||
|         if events.is_readable() && self.0.can_read_more(was_init) { |         if events.is_readable() && self.0.can_read_more(was_init) { | ||||||
|             return self.ready(events, scope); |             return ReadyResult::Continue(self); | ||||||
|         } |         } | ||||||
|  |  | ||||||
|         trace!("scope.reregister({:?})", events); |         trace!("scope.reregister({:?})", events); | ||||||
|         match scope.reregister(&self.0.transport, events, PollOpt::level()) { |         match scope.reregister(&self.0.transport, events, PollOpt::level()) { | ||||||
|             Ok(..) => { |             Ok(..) => { | ||||||
|                 let timeout = self.0.state.timeout(); |                 let timeout = self.0.state.timeout(); | ||||||
|                 Some((self, timeout)) |                 ReadyResult::Done(Some((self, timeout))) | ||||||
|             }, |             }, | ||||||
|             Err(e) => { |             Err(e) => { | ||||||
|                 trace!("error reregistering: {:?}", e); |                 trace!("error reregistering: {:?}", e); | ||||||
|                 self.0.on_error(e.into(), &**scope); |                 self.0.on_error(e.into(), &**scope); | ||||||
|                 None |                 ReadyResult::Done(None) | ||||||
|             } |             } | ||||||
|         } |         } | ||||||
|     } |     } | ||||||
| @@ -607,14 +617,27 @@ impl<K: Key, T: Transport, H: MessageHandler<T>> Conn<K, T, H> { | |||||||
|             trace!("woke up with {:?}", next); |             trace!("woke up with {:?}", next); | ||||||
|             self.0.state.update(next, &**scope); |             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<F>(mut self, scope: &mut Scope<F>) -> Option<(Self, Option<Duration>)> |     pub fn timeout<F>(mut self, scope: &mut Scope<F>) -> Option<(Self, Option<Duration>)> | ||||||
|     where F: MessageHandlerFactory<K, T, Output=H> { |     where F: MessageHandlerFactory<K, T, Output=H> { | ||||||
|         //TODO: check if this was a spurious timeout? |         //TODO: check if this was a spurious timeout? | ||||||
|         self.0.on_error(::Error::Timeout, &**scope); |         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) { |     fn on_remove(self) { | ||||||
|   | |||||||
| @@ -17,7 +17,7 @@ use version::HttpVersion::{Http10, Http11}; | |||||||
| #[cfg(feature = "serde-serialization")] | #[cfg(feature = "serde-serialization")] | ||||||
| use serde::{Deserialize, Deserializer, Serialize, Serializer}; | 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; | mod buffer; | ||||||
| pub mod channel; | pub mod channel; | ||||||
|   | |||||||
| @@ -14,7 +14,7 @@ use rotor::{self, Scope}; | |||||||
| pub use self::request::Request; | pub use self::request::Request; | ||||||
| pub use self::response::Response; | pub use self::response::Response; | ||||||
|  |  | ||||||
| use http::{self, Next}; | use http::{self, Next, ReadyResult}; | ||||||
|  |  | ||||||
| pub use net::{Accept, HttpListener, HttpsListener}; | pub use net::{Accept, HttpListener, HttpsListener}; | ||||||
| use net::{SslServer, Transport}; | use net::{SslServer, Transport}; | ||||||
| @@ -248,13 +248,21 @@ where A: Accept, | |||||||
|                 } |                 } | ||||||
|             }, |             }, | ||||||
|             ServerFsm::Conn(conn) => { |             ServerFsm::Conn(conn) => { | ||||||
|                 match conn.ready(events, scope) { |                 let mut conn = Some(conn); | ||||||
|                     Some((conn, None)) => rotor::Response::ok(ServerFsm::Conn(conn)), |                 loop { | ||||||
|                     Some((conn, Some(dur))) => { |                     match conn.take().unwrap().ready(events, scope) { | ||||||
|                         rotor::Response::ok(ServerFsm::Conn(conn)) |                         ReadyResult::Continue(c) => conn = Some(c), | ||||||
|                             .deadline(scope.now() + dur) |                         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() |  | ||||||
|                 } |                 } | ||||||
|             } |             } | ||||||
|         } |         } | ||||||
|   | |||||||
		Reference in New Issue
	
	Block a user