From 20591e882e64f3190cfd24f377cea66e07c3d68c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Sat, 22 Oct 2016 20:06:30 +0200 Subject: [PATCH] Return old-ish content even when syncing (#2757) --- dapps/src/apps/fetcher.rs | 39 +++++++++++++++++++++----------------- dapps/src/tests/fetch.rs | 30 ++++++++++++++++++++++++++++- dapps/src/tests/helpers.rs | 13 +++++++++---- 3 files changed, 60 insertions(+), 22 deletions(-) diff --git a/dapps/src/apps/fetcher.rs b/dapps/src/apps/fetcher.rs index 9d66276ea..210c6b180 100644 --- a/dapps/src/apps/fetcher.rs +++ b/dapps/src/apps/fetcher.rs @@ -69,6 +69,15 @@ impl ContentFetcher { } } + fn still_syncing() -> Box { + Box::new(ContentHandler::error( + StatusCode::ServiceUnavailable, + "Sync In Progress", + "Your node is still syncing. We cannot resolve any content before it's fully synced.", + Some("Refresh") + )) + } + #[cfg(test)] fn set_status(&self, content_id: &str, status: ContentStatus) { self.cache.lock().insert(content_id.to_owned(), status); @@ -84,12 +93,10 @@ impl ContentFetcher { } // fallback to resolver if let Ok(content_id) = content_id.from_hex() { - // if app_id is valid, but we are syncing always return true. - if self.sync.is_major_importing() { - return true; - } // else try to resolve the app_id - self.resolver.resolve(content_id).is_some() + let has_content = self.resolver.resolve(content_id).is_some(); + // if there is content or we are syncing return true + has_content || self.sync.is_major_importing() } else { false } @@ -99,28 +106,19 @@ impl ContentFetcher { let mut cache = self.cache.lock(); let content_id = path.app_id.clone(); - if self.sync.is_major_importing() { - return Box::new(ContentHandler::error( - StatusCode::ServiceUnavailable, - "Sync In Progress", - "Your node is still syncing. We cannot resolve any content before it's fully synced.", - Some("Refresh") - )); - } - let (new_status, handler) = { let status = cache.get(&content_id); match status { - // Just server dapp + // Just serve the content Some(&mut ContentStatus::Ready(ref endpoint)) => { (None, endpoint.to_async_handler(path, control)) }, - // App is already being fetched + // Content is already being fetched Some(&mut ContentStatus::Fetching(ref fetch_control)) => { trace!(target: "dapps", "Content fetching in progress. Waiting..."); (None, fetch_control.to_handler(control)) }, - // We need to start fetching app + // We need to start fetching the content None => { trace!(target: "dapps", "Content unavailable. Fetching... {:?}", content_id); let content_hex = content_id.from_hex().expect("to_handler is called only when `contains` returns true."); @@ -141,6 +139,10 @@ impl ContentFetcher { }; match content { + // Don't serve dapps if we are still syncing (but serve content) + Some(URLHintResult::Dapp(_)) if self.sync.is_major_importing() => { + (None, Self::still_syncing()) + }, Some(URLHintResult::Dapp(dapp)) => { let (handler, fetch_control) = ContentFetcherHandler::new( dapp.url(), @@ -170,6 +172,9 @@ impl ContentFetcher { (Some(ContentStatus::Fetching(fetch_control)), Box::new(handler) as Box) }, + None if self.sync.is_major_importing() => { + (None, Self::still_syncing()) + }, None => { // This may happen when sync status changes in between // `contains` and `to_handler` diff --git a/dapps/src/tests/fetch.rs b/dapps/src/tests/fetch.rs index 9f02e3385..1cabca5cb 100644 --- a/dapps/src/tests/fetch.rs +++ b/dapps/src/tests/fetch.rs @@ -14,7 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Parity. If not, see . -use tests::helpers::{serve_with_registrar, request, assert_security_headers}; +use tests::helpers::{serve_with_registrar, serve_with_registrar_and_sync, request, assert_security_headers}; #[test] fn should_resolve_dapp() { @@ -37,3 +37,31 @@ fn should_resolve_dapp() { assert_security_headers(&response.headers); } +#[test] +fn should_return_503_when_syncing_but_should_make_the_calls() { + // given + let (server, registrar) = serve_with_registrar_and_sync(); + { + let mut responses = registrar.responses.lock(); + let res1 = responses.get(0).unwrap().clone(); + let res2 = responses.get(1).unwrap().clone(); + // Registrar will be called twice - fill up the responses. + responses.push(res1); + responses.push(res2); + } + + // when + let response = request(server, + "\ + GET / HTTP/1.1\r\n\ + Host: 1472a9e190620cdf6b31f383373e45efcfe869a820c91f9ccd7eb9fb45e4985d.parity\r\n\ + Connection: close\r\n\ + \r\n\ + " + ); + + // then + assert_eq!(response.status, "HTTP/1.1 503 Service Unavailable".to_owned()); + assert_eq!(registrar.calls.lock().len(), 4); + assert_security_headers(&response.headers); +} diff --git a/dapps/src/tests/helpers.rs b/dapps/src/tests/helpers.rs index 0069dc31b..1fa2e777a 100644 --- a/dapps/src/tests/helpers.rs +++ b/dapps/src/tests/helpers.rs @@ -69,12 +69,13 @@ fn init_logger() { } } -pub fn init_server(hosts: Option>) -> (Server, Arc) { +pub fn init_server(hosts: Option>, is_syncing: bool) -> (Server, Arc) { init_logger(); let registrar = Arc::new(FakeRegistrar::new()); let mut dapps_path = env::temp_dir(); dapps_path.push("non-existent-dir-to-prevent-fs-files-from-loading"); let mut builder = ServerBuilder::new(dapps_path.to_str().unwrap().into(), registrar.clone()); + builder.with_sync_status(Arc::new(move || is_syncing)); builder.with_signer_port(Some(SIGNER_PORT)); ( builder.start_unsecured_http(&"127.0.0.1:0".parse().unwrap(), hosts).unwrap(), @@ -93,15 +94,19 @@ pub fn serve_with_auth(user: &str, pass: &str) -> Server { } pub fn serve_hosts(hosts: Option>) -> Server { - init_server(hosts).0 + init_server(hosts, false).0 } pub fn serve_with_registrar() -> (Server, Arc) { - init_server(None) + init_server(None, false) +} + +pub fn serve_with_registrar_and_sync() -> (Server, Arc) { + init_server(None, true) } pub fn serve() -> Server { - init_server(None).0 + init_server(None, false).0 } pub fn request(server: Server, request: &str) -> http_client::Response {