Removing on_done

dapps/src/apps/fetcher.rs

@@ -120,7 +120,7 @@ impl<R: URLHint> ContentFetcher<R> {
 				// 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))
+					(None, fetch_control.to_async_handler(path, control))
 				},
 				// We need to start fetching the content
 				None => {
@@ -129,11 +129,12 @@ impl<R: URLHint> ContentFetcher<R> {
 					let content = self.resolver.resolve(content_hex);
 
 					let cache = self.cache.clone();
-					let on_done = move |id: String, result: Option<LocalPageEndpoint>| {
+					let id = content_id.clone();
+					let on_done = move |result: Option<LocalPageEndpoint>| {
 						let mut cache = cache.lock();
 						match result {
 							Some(endpoint) => {
-								cache.insert(id, ContentStatus::Ready(endpoint));
+								cache.insert(id.clone(), ContentStatus::Ready(endpoint));
 							},
 							// In case of error
 							None => {
@@ -248,43 +249,45 @@ struct ContentInstaller {
 	id: String,
 	mime: String,
 	content_path: PathBuf,
-	on_done: Box<Fn(String, Option<LocalPageEndpoint>) + Send>,
+	on_done: Box<Fn(Option<LocalPageEndpoint>) + Send>,
 }
 
 impl ContentValidator for ContentInstaller {
 	type Error = ValidationError;
 
 	fn validate_and_install(&self, path: PathBuf) -> Result<(String, LocalPageEndpoint), ValidationError> {
-		// Create dir
+		let validate = || {
-		try!(fs::create_dir_all(&self.content_path));
+			// Create dir
+			try!(fs::create_dir_all(&self.content_path));
 
-		// Validate hash
+			// Validate hash
-		let mut file_reader = io::BufReader::new(try!(fs::File::open(&path)));
+			let mut file_reader = io::BufReader::new(try!(fs::File::open(&path)));
-		let hash = try!(sha3(&mut file_reader));
+			let hash = try!(sha3(&mut file_reader));
-		let id = try!(self.id.as_str().parse().map_err(|_| ValidationError::InvalidContentId));
+			let id = try!(self.id.as_str().parse().map_err(|_| ValidationError::InvalidContentId));
-		if id != hash {
+			if id != hash {
-			return Err(ValidationError::HashMismatch {
+				return Err(ValidationError::HashMismatch {
-				expected: id,
+					expected: id,
-				got: hash,
+					got: hash,
-			});
+				});
-		}
+			}
 
-		// And prepare path for a file
+			// And prepare path for a file
-		let filename = path.file_name().expect("We always fetch a file.");
+			let filename = path.file_name().expect("We always fetch a file.");
-		let mut content_path = self.content_path.clone();
+			let mut content_path = self.content_path.clone();
-		content_path.push(&filename);
+			content_path.push(&filename);
 
-		if content_path.exists() {
+			if content_path.exists() {
-			try!(fs::remove_dir_all(&content_path))
+				try!(fs::remove_dir_all(&content_path))
-		}
+			}
 
-		try!(fs::copy(&path, &content_path));
+			try!(fs::copy(&path, &content_path));
+			Ok(LocalPageEndpoint::single_file(content_path, self.mime.clone(), PageCache::Enabled))
+		};
 
-		Ok((self.id.clone(), LocalPageEndpoint::single_file(content_path, self.mime.clone(), PageCache::Enabled)))
+		// Make sure to always call on_done (even in case of errors)!
-	}
+		let result = validate();
-
+		(self.on_done)(result.as_ref().ok().cloned());
-	fn done(&self, endpoint: Option<LocalPageEndpoint>) {
+		result.map(|endpoint| (self.id.clone(), endpoint))
-		(self.on_done)(self.id.clone(), endpoint)
 	}
 }
 
@@ -292,7 +295,7 @@ impl ContentValidator for ContentInstaller {
 struct DappInstaller {
 	id: String,
 	dapps_path: PathBuf,
-	on_done: Box<Fn(String, Option<LocalPageEndpoint>) + Send>,
+	on_done: Box<Fn(Option<LocalPageEndpoint>) + Send>,
 	embeddable_on: Option<(String, u16)>,
 }
 
@@ -333,67 +336,67 @@ impl ContentValidator for DappInstaller {
 
 	fn validate_and_install(&self, app_path: PathBuf) -> Result<(String, LocalPageEndpoint), ValidationError> {
 		trace!(target: "dapps", "Opening dapp bundle at {:?}", app_path);
-		let mut file_reader = io::BufReader::new(try!(fs::File::open(app_path)));
+		let validate = || {
-		let hash = try!(sha3(&mut file_reader));
+			let mut file_reader = io::BufReader::new(try!(fs::File::open(app_path)));
-		let id = try!(self.id.as_str().parse().map_err(|_| ValidationError::InvalidContentId));
+			let hash = try!(sha3(&mut file_reader));
-		if id != hash {
+			let id = try!(self.id.as_str().parse().map_err(|_| ValidationError::InvalidContentId));
-			return Err(ValidationError::HashMismatch {
+			if id != hash {
-				expected: id,
+				return Err(ValidationError::HashMismatch {
-				got: hash,
+					expected: id,
-			});
+					got: hash,
-		}
+				});
-		let file = file_reader.into_inner();
+			}
-		// Unpack archive
+			let file = file_reader.into_inner();
-		let mut zip = try!(zip::ZipArchive::new(file));
+			// Unpack archive
-		// First find manifest file
+			let mut zip = try!(zip::ZipArchive::new(file));
-		let (mut manifest, manifest_dir) = try!(Self::find_manifest(&mut zip));
+			// First find manifest file
-		// Overwrite id to match hash
+			let (mut manifest, manifest_dir) = try!(Self::find_manifest(&mut zip));
-		manifest.id = self.id.clone();
+			// Overwrite id to match hash
+			manifest.id = self.id.clone();
 
-		let target = self.dapp_target_path(&manifest);
+			let target = self.dapp_target_path(&manifest);
 
-		// Remove old directory
+			// Remove old directory
-		if target.exists() {
+			if target.exists() {
-			warn!(target: "dapps", "Overwriting existing dapp: {}", manifest.id);
+				warn!(target: "dapps", "Overwriting existing dapp: {}", manifest.id);
-			try!(fs::remove_dir_all(target.clone()));
+				try!(fs::remove_dir_all(target.clone()));
-		}
+			}
 
-		// Unpack zip
+			// Unpack zip
-		for i in 0..zip.len() {
+			for i in 0..zip.len() {
-			let mut file = try!(zip.by_index(i));
+				let mut file = try!(zip.by_index(i));
-			// TODO [todr] Check if it's consistent on windows.
+				// TODO [todr] Check if it's consistent on windows.
-			let is_dir = file.name().chars().rev().next() == Some('/');
+				let is_dir = file.name().chars().rev().next() == Some('/');
 
-			let file_path = PathBuf::from(file.name());
+				let file_path = PathBuf::from(file.name());
-			let location_in_manifest_base = file_path.strip_prefix(&manifest_dir);
+				let location_in_manifest_base = file_path.strip_prefix(&manifest_dir);
-			// Create files that are inside manifest directory
+				// Create files that are inside manifest directory
-			if let Ok(location_in_manifest_base) = location_in_manifest_base {
+				if let Ok(location_in_manifest_base) = location_in_manifest_base {
-				let p = target.join(location_in_manifest_base);
+					let p = target.join(location_in_manifest_base);
-				// Check if it's a directory
+					// Check if it's a directory
-				if is_dir {
+					if is_dir {
-					try!(fs::create_dir_all(p));
+						try!(fs::create_dir_all(p));
-				} else {
+					} else {
-					let mut target = try!(fs::File::create(p));
+						let mut target = try!(fs::File::create(p));
-					try!(io::copy(&mut file, &mut target));
+						try!(io::copy(&mut file, &mut target));
+					}
 				}
 			}
-		}
 
-		// Write manifest
+			// Write manifest
-		let manifest_str = try!(serialize_manifest(&manifest).map_err(ValidationError::ManifestSerialization));
+			let manifest_str = try!(serialize_manifest(&manifest).map_err(ValidationError::ManifestSerialization));
-		let manifest_path = target.join(MANIFEST_FILENAME);
+			let manifest_path = target.join(MANIFEST_FILENAME);
-		let mut manifest_file = try!(fs::File::create(manifest_path));
+			let mut manifest_file = try!(fs::File::create(manifest_path));
-		try!(manifest_file.write_all(manifest_str.as_bytes()));
+			try!(manifest_file.write_all(manifest_str.as_bytes()));
+			// Create endpoint
+			let endpoint = LocalPageEndpoint::new(target, manifest.clone().into(), PageCache::Enabled, self.embeddable_on.clone());
+			// Return modified app manifest
+			Ok(endpoint)
+		};
 
-		// Create endpoint
+		let result = validate();
-		let app = LocalPageEndpoint::new(target, manifest.clone().into(), PageCache::Enabled, self.embeddable_on.clone());
+		(self.on_done)(result.as_ref().ok().cloned());
-
+		result.map(|endpoint| (self.id.clone(), endpoint))
-		// Return modified app manifest
-		Ok((manifest.id.clone(), app))
-	}
-
-	fn done(&self, endpoint: Option<LocalPageEndpoint>) {
-		(self.on_done)(self.id.clone(), endpoint)
 	}
 }
 

dapps/src/handlers/fetch.rs

@@ -29,6 +29,7 @@ use hyper::{server, Decoder, Encoder, Next, Method, Control};
 use hyper::net::HttpStream;
 use hyper::status::StatusCode;
 
+use endpoint::EndpointPath;
 use handlers::{ContentHandler, Redirection, extract_url};
 use page::LocalPageEndpoint;
 
@@ -45,7 +46,6 @@ pub trait ContentValidator {
 	type Error: fmt::Debug + fmt::Display;
 
 	fn validate_and_install(&self, app: PathBuf) -> Result<(String, LocalPageEndpoint), Self::Error>;
-	fn done(&self, Option<LocalPageEndpoint>);
 }
 
 pub struct FetchControl {
@@ -88,7 +88,9 @@ impl FetchControl {
 		self.abort.store(true, Ordering::SeqCst);
 	}
 
-	pub fn to_handler(&self, control: Control) -> Box<server::Handler<HttpStream> + Send> {
+	pub fn to_async_handler(&self, path: EndpointPath, control: Control) -> Box<server::Handler<HttpStream> + Send> {
+		// TODO [ToDr] We should be able to pass EndpointPath to handler as well
+		// (request may be coming from different domain, etc)
 		let (tx, rx) = mpsc::channel();
 		self.listeners.lock().push((control, tx));
 
@@ -141,25 +143,13 @@ pub struct ContentFetcherHandler<H: ContentValidator> {
 	embeddable_on: Option<(String, u16)>,
 }
 
-impl<H: ContentValidator> Drop for ContentFetcherHandler<H> {
-	fn drop(&mut self) {
-		let result = match self.status {
-			FetchState::Done(_, ref result, _) => Some(result.clone()),
-			_ => None,
-		};
-		self.installer.done(result);
-	}
-}
-
 impl<H: ContentValidator> ContentFetcherHandler<H> {
-
 	pub fn new(
 		url: String,
 		control: Control,
 		handler: H,
 		embeddable_on: Option<(String, u16)>,
 	) -> (Self, Arc<FetchControl>) {
-
 		let fetch_control = Arc::new(FetchControl::default());
 		let client = Client::default();
 		let handler = ContentFetcherHandler {