Improved mechanism for re-using already loaded / loading resources. (#78)

* Improved mechanism for re-using already loaded / loading resources.

* Rewrote loading logic:

1. `requestResource` executes the request for the resource, and saves the response. Processing multiple times is prevented by maintaining `respondedResourcePromises`.
2. `loadResource` calls `requestResource` and saves the resource to disk. Processing multiple times is prevented by maintaining `loadedResourcePromises`.
3. To prevent deadlocks `file-handlers/html.js` only waits for `requestResource`, not for `loadResource`.
4. To determine when the scraper is finished `loadedResourcePromises` is used.

* Fix bluebird warning

* Code style fixes

* Fixed bug

* Style fix

* Set filename when response was recieved, to always use the responded filename

* Cleanup

* Update scraper.js

Added `waitForLoad` explanation.

Author: Mark Lagendijk

lib/file-handlers/html.js

@@ -79,12 +79,16 @@ function loadImgSrcsetResource (context, parentResource, childResourceHtmlData)
 		var childResourceUrl = utils.getUrl(parentResource.getUrl(), imgScrsetPart.url);
 		var childResource = parentResource.createChild(childResourceUrl);
 		childResource.setHtmlData(childResourceHtmlData);
-
-		return context.loadResource(childResource).then(function updateSrcsetPart (loadedResource) {
-			if(loadedResource){
-				parentResource.updateChild(childResource, loadedResource);
-				imgScrsetPart.url = loadedResource.getFilename();
+		
+		return context.requestResource(childResource).then(function updateSrcsetPart (respondedResource) {
+			if(respondedResource){
+				parentResource.updateChild(childResource, respondedResource);
+				imgScrsetPart.url = respondedResource.getFilename();
+
+				// Do not wait for loadResource here, to prevent deadlock, see scraper.waitForLoad
+				context.loadResource(childResource);
 			}
+			return null; // Prevent Bluebird warnings
 		});
 	}).then(function updateSrcset () {
 		return Promise.resolve(srcset.stringify(imgScrsetParts));
@@ -102,25 +106,28 @@ function loadGeneralResource (context, parentResource, childResourceHtmlData) {
 	var attr = childResourceHtmlData.attributeValue;
 
 	var resourceUrl = utils.getUrl(parentResource.getUrl(), attr);
-	var htmlResource = parentResource.createChild(resourceUrl);
-	htmlResource.setHtmlData(childResourceHtmlData);
+	var childResource = parentResource.createChild(resourceUrl);
+	childResource.setHtmlData(childResourceHtmlData);
 
-	return context.loadResource(htmlResource).then(function handleLoadedSource (loadedResource) {
-		if(!loadedResource){
+	return context.requestResource(childResource).then(function handleLoadedSource (respondedResource) {
+		if(!respondedResource){
 			return attr;
 		}
-		parentResource.updateChild(htmlResource, loadedResource);
+		parentResource.updateChild(childResource, respondedResource);
 
-		var relativePath = utils.getRelativePath(parentResource.getFilename(), loadedResource.getFilename());
+		var relativePath = utils.getRelativePath(parentResource.getFilename(), respondedResource.getFilename());
 		if(context.options.prettifyUrls){
 			relativePath = relativePath.replace(context.options.defaultFilename, '');
 		}
 		var hash = utils.getHashFromUrl(attr);
 
-		if (hash && loadedResource.isHtml()) {
+		if (hash && respondedResource.isHtml()) {
 			relativePath = relativePath.concat(hash);
 		}
 
+		// Do not wait for loadResource here, to prevent deadlock, scraper.waitForLoad
+		context.loadResource(childResource);
+
 		return relativePath;
 	});
 }

lib/filename-generators/by-type.js

@@ -3,8 +3,8 @@ var path = require('path');
 var utils = require('../utils.js');
 var defaultExtensions = require('../config/resource-extensions-by-type');
 
-module.exports = function generateFilename (resource, options, loadedResources) {
-	var occupiedFilenames = getOccupiedFilenames(loadedResources, options);
+module.exports = function generateFilename (resource, options, occupiedFileNames) {
+	var occupiedNames = getSubDirectoryNames(options).concat(occupiedFileNames);
 
 	var filename = getFilenameForResource(resource, options);
 	var extension = utils.getFilenameExtension(filename);
@@ -14,7 +14,7 @@ module.exports = function generateFilename (resource, options, loadedResources)
 	var basename = path.basename(filename, extension);
 	var index = 1;
 
-	while (_.includes(occupiedFilenames, currentFilename)) {
+	while (_.includes(occupiedNames, currentFilename)) {
 		currentFilename = path.join(directory, basename + '_' + index + extension);
 		index++;
 	}
@@ -37,10 +37,8 @@ function getFilenameForResource (resource, options) {
 	return filename;
 }
 
-function getOccupiedFilenames (loadedResources, options) {
-	var subdirectories = _.map(options.subdirectories, function getDirectory (directory) { return directory.directory; });
-	var loadedFiles = _.map(loadedResources, function getFileName (resource) { return resource.getFilename(); });
-	return subdirectories.concat(loadedFiles);
+function getSubDirectoryNames (options) {
+	return _.map(options.subdirectories, function getDirectory (directory) { return directory.directory; });
 }
 
 function getDirectoryByExtension (extension, options) {

lib/scraper.js

@@ -14,7 +14,7 @@ var Resource = require('./resource');
 
 var getFilenameGenerator = require('./filename-generators/filename-generator-getter');
 var makeRequest = require('./request');
-var compareUrls = require('compare-urls');
+var normalizeUrl = require('normalize-url');
 
 var loadHtml = require('./file-handlers/html');
 var loadCss = require('./file-handlers/css');
@@ -26,24 +26,49 @@ function loadHtmlAndCss (context, po) {
 
 function Scraper (options) {
 	this.originalResources = [];
+	this.occupiedFileNames = [];
 	this.loadedResources = [];
+	this.respondedResourcePromises = {};
+	this.loadedResourcePromises = {};
 
 	this.options = _.extend({}, defaults, options);
 	this.options.request = _.extend({}, defaults.request, options.request);
 	this.options.directory = path.resolve(process.cwd(), this.options.directory || '');
 	this.options.filenameGenerator = getFilenameGenerator(this.options.filenameGenerator);
 }
 
-Scraper.prototype.getLoadedResource = function getLoadedResource (resource) {
-	return _.find(this.loadedResources, function checkUrlsEqual (lr) {
-		return compareUrls(resource.getUrl(), lr.getUrl());
-	});
+Scraper.prototype.addOccupiedFileName = function addOccupiedFileName (filename) {
+	this.occupiedFileNames.push(filename);
+};
+
+Scraper.prototype.getOccupiedFileNames = function getOccupiedFileNames () {
+	return this.occupiedFileNames;
 };
 
 Scraper.prototype.addLoadedResource = function addLoadedResource (resource) {
 	this.loadedResources.push(resource);
 };
 
+Scraper.prototype.addRespondedResourcePromise = function addRespondedResourcePromise (url, promise) {
+	url = normalizeUrl(url);
+	this.respondedResourcePromises[url] = promise;
+};
+
+Scraper.prototype.getRespondedResourcePromise = function getRespondedResourcePromise (url) {
+	url = normalizeUrl(url);
+	return this.respondedResourcePromises[url];
+};
+
+Scraper.prototype.addLoadedResourcePromise = function addLoadedResourcePromise (url, promise) {
+	url = normalizeUrl(url);
+	this.loadedResourcePromises[url] = promise;
+};
+
+Scraper.prototype.getLoadedResourcePromise = function getLoadedResourcePromise (url) {
+	url = normalizeUrl(url);
+	return this.loadedResourcePromises[url];
+};
+
 Scraper.prototype.getHtmlSources = function getHtmlSources () {
 	return this.options.sources;
 };
@@ -53,46 +78,81 @@ Scraper.prototype.getResourceHandler = function getHandler (resource) {
 	var depth = resource.getDepth();
 	var depthGreaterThanMax = self.options.maxDepth && depth >= self.options.maxDepth;
 
-	switch (true) {
-		case depthGreaterThanMax: return _.noop;
-		case resource.isCss(): return loadCss;
-		case resource.isHtml(): return loadHtmlAndCss;
-		default: return _.noop;
+	switch(true) {
+		case depthGreaterThanMax:
+			return _.noop;
+		case resource.isCss():
+			return loadCss;
+		case resource.isHtml():
+			return loadHtmlAndCss;
+		default:
+			return _.noop;
 	}
 };
 
 Scraper.prototype.loadResource = function loadResource (resource) {
 	var self = this;
+	var url = resource.getUrl();
 
-	if(!self.options.urlFilter(resource.url)){
+	if(!self.options.urlFilter(url)) {
 		return Promise.resolve(null);
 	}
 
-	// try to find already loaded
-	var loaded = self.getLoadedResource(resource);
+	var loadedResourcePromise = self.getLoadedResourcePromise(url);
+	if(loadedResourcePromise) {
+		return loadedResourcePromise;
+	}
 
-	if (!loaded) {
-		var url = resource.getUrl();
-		var filename = self.options.filenameGenerator(resource, self.options, self.loadedResources);
-		resource.setFilename(filename);
+	var respondedResourcePromise = self.requestResource(resource);
+
+	loadedResourcePromise = respondedResourcePromise.then(function handleResponse (resource) {
+		return Promise.resolve()
+			.then(function handleResource () {
+				var resourceHandler = self.getResourceHandler(resource);
+				return resourceHandler(self, resource);
+			}).then(function fileHandled () {
+				var filename = path.join(self.options.directory, resource.getFilename());
+				var text = resource.getText();
+				return outputFileAsync(filename, text, { encoding: 'binary' });
+			}).then(function fileSaved () {
+				self.addLoadedResourcePromise(resource.getUrl(), loadedResourcePromise);
+				self.addLoadedResource(resource);
+				return resource;
+			});
+	});
+	self.addLoadedResourcePromise(url, loadedResourcePromise);
+
+	return loadedResourcePromise;
+};
+
+Scraper.prototype.requestResource = function requestResource (resource) {
+	var self = this;
+	var url = resource.getUrl();
+
+	if(!self.options.urlFilter(url)) {
+		return Promise.resolve(null);
+	}
 
-		self.addLoadedResource(resource);
-
-		// Request -> processing -> save to fs
-		return self.makeRequest(url).then(function requestCompleted (data) {
-			resource.setUrl(data.url);  // Url may be changed in redirects
-			resource.setText(data.body);
-			var handleFile = self.getResourceHandler(resource);
-			return handleFile(self, resource);
-		}).then(function fileHandled () {
-			var filename = path.join(self.options.directory, resource.getFilename());
-			var text = resource.getText();
-			return outputFileAsync(filename, text, { encoding: 'binary' });
-		}).then(function fileSaved () {
-			return Promise.resolve(resource);
-		});
+	var respondedResourcePromise = self.getRespondedResourcePromise(url);
+	if(respondedResourcePromise) {
+		return respondedResourcePromise;
 	}
-	return Promise.resolve(loaded);
+
+	var promise = self.makeRequest(url).then(function requestCompleted (data) {
+		// Url may be changed in redirects
+		resource.setUrl(data.url);
+		
+		var filename = self.options.filenameGenerator(resource, self.options, self.getOccupiedFileNames());
+		resource.setFilename(filename);
+		self.addOccupiedFileName(filename);
+		self.addRespondedResourcePromise(data.url, promise);
+
+		resource.setText(data.body);
+		return resource;
+	});
+	self.addRespondedResourcePromise(url, promise);
+
+	return promise;
 };
 
 Scraper.prototype.validate = function validate () {
@@ -118,7 +178,7 @@ Scraper.prototype.prepare = function prepare () {
 		return new Resource(url, filename);
 	});
 
-	if (self.options.recursive) {
+	if(self.options.recursive) {
 		self.options.sources = _.union(self.options.sources, recursiveSources);
 	}
 
@@ -127,9 +187,28 @@ Scraper.prototype.prepare = function prepare () {
 
 Scraper.prototype.load = function load () {
 	var self = this;
-	return Promise.map(self.originalResources, function loadResource (resource) {
-		return self.loadResource(resource);
+	_.forEach(self.originalResources, function loadResource (resource) {
+		self.loadResource(resource);
 	});
+	return self.waitForLoad(0);
+};
+
+// Returns a promise which gets resolved when all resources are loaded.
+// Determines whether loading is done by:
+// 1. Waiting until all loadedResourcePromises are resolved.
+// 2. Recursing itself if any new loadedResourcePromises (promises for the loading of childResources) where added during this time. If not, loading is done.
+Scraper.prototype.waitForLoad = function waitForLoad (previousCount) {
+	var self = this;
+	var count = _.size(self.loadedResourcePromises);
+	if(count === previousCount) {
+		return Promise.all(_.map(self.originalResources, function getLoadedResource (resource) {
+			return self.getLoadedResourcePromise(resource.getUrl());
+		}));
+	} else{
+		return Promise
+			.all(_.toArray(self.loadedResourcePromises))
+			.then(self.waitForLoad.bind(self, count));
+	}
 };
 
 Scraper.prototype.errorCleanup = function errorCleanup (error) {

package.json

@@ -34,10 +34,10 @@
   "dependencies": {
     "bluebird": "^3.0.1",
     "cheerio": "0.20.0",
-    "compare-urls": "^1.0.0",
     "css-url-parser": "^1.0.0",
     "fs-extra": "^0.30.0",
     "lodash": "^4.11.1",
+    "normalize-url": "^1.5.3",
     "request": "^2.42.0",
     "srcset": "^1.0.0"
   },