From ead038c34f514d707ee663bf35084bb80139957c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Tisza?= Date: Mon, 3 Mar 2014 19:04:01 +0000 Subject: [PATCH] Add rejection logging to providers Make sure it is easy to debug when one of the promises rejects (and causes the whole promise chain to fail). I'm not really happy with this, but still seemed better than adding the same boilerplate error logging code to each provider one-by-one. Change-Id: Idd2b638f012ef2ff250e350e2f6a60bb8b81899b Mingle: https://wikimedia.mingle.thoughtworks.com/projects/multimedia/cards/268 --- resources/mmv/provider/mmv.provider.Api.js | 27 +++++++++++++++++++ .../mmv/provider/mmv.provider.FileRepoInfo.js | 8 +++--- .../mmv/provider/mmv.provider.GlobalUsage.js | 11 +++----- resources/mmv/provider/mmv.provider.Image.js | 4 ++- .../mmv/provider/mmv.provider.ImageInfo.js | 17 +++++------- .../mmv/provider/mmv.provider.ImageUsage.js | 15 +++++------ .../provider/mmv.provider.ThumbnailInfo.js | 8 +++--- .../mmv/provider/mmv.provider.UserInfo.js | 8 +++--- 8 files changed, 56 insertions(+), 42 deletions(-) diff --git a/resources/mmv/provider/mmv.provider.Api.js b/resources/mmv/provider/mmv.provider.Api.js index 8e0d9d3a1..67bc66588 100644 --- a/resources/mmv/provider/mmv.provider.Api.js +++ b/resources/mmv/provider/mmv.provider.Api.js @@ -46,6 +46,33 @@ this.cache = {}; } + /** + * Wraps a caching layer around a function returning a promise; if getCachedPromise has been + * called with the same key already, it will return the previous result. + * + * Since it is the promise and not the API response that gets cached, this method can ensure + * that there are no race conditions and multiple calls to the same resource: even if the + * request is still in progress, separate calls (with the same key) to getCachedPromise will + * share on the same promise object. + * The promise is cached even if it is rejected, so if the API request fails, all later calls + * to getCachedPromise will fail immediately without retrying the request. + * + * @param {string} key cache key + * @param {function(): jQuery.Promise} getPromise a function to get the promise on cache miss + */ + Api.prototype.getCachedPromise = function( key, getPromise ) { + var provider = this; + + if ( !this.cache[key] ) { + this.cache[key] = getPromise(); + this.cache[key].fail( function ( error ) { + // constructor.name is usually not reliable in inherited classes, but OOjs fixes that + mw.log( provider.constructor.name + ' provider failed to load: ', error ); + } ); + } + return this.cache[key]; + }; + /** * Pulls an error message out of an API response. * @param {Object} data diff --git a/resources/mmv/provider/mmv.provider.FileRepoInfo.js b/resources/mmv/provider/mmv.provider.FileRepoInfo.js index 283d40233..3b0a548fc 100644 --- a/resources/mmv/provider/mmv.provider.FileRepoInfo.js +++ b/resources/mmv/provider/mmv.provider.FileRepoInfo.js @@ -37,8 +37,8 @@ FileRepoInfo.prototype.get = function() { var provider = this; - if ( !this.cache['*'] ) { - this.cache['*'] = this.api.get( { + return this.getCachedPromise( '*', function () { + return provider.api.get( { action: 'query', meta: 'filerepoinfo', format: 'json' @@ -51,9 +51,7 @@ } ); return reposHash; } ); - } - - return this.cache['*']; + } ); }; mw.mmv.provider.FileRepoInfo = FileRepoInfo; diff --git a/resources/mmv/provider/mmv.provider.GlobalUsage.js b/resources/mmv/provider/mmv.provider.GlobalUsage.js index 15df554ec..fca4198b1 100644 --- a/resources/mmv/provider/mmv.provider.GlobalUsage.js +++ b/resources/mmv/provider/mmv.provider.GlobalUsage.js @@ -53,7 +53,6 @@ */ GlobalUsage.prototype.get = function( file ) { var provider = this, - cacheKey = file.getPrefixedDb(), fileUsage; if ( this.options.doNotUseApi ) { @@ -63,14 +62,14 @@ return $.Deferred().resolve( fileUsage ); } - if ( !this.cache[cacheKey] ) { - this.cache[cacheKey] = this.api.get( { + return this.getCachedPromise( file.getPrefixedDb(), function () { + return provider.api.get( { action: 'query', prop: 'globalusage', titles: file.getPrefixedDb(), guprop: ['url', 'namespace'], gufilterlocal: 1, - gulimit: this.options.apiLimit, + gulimit: provider.options.apiLimit, format: 'json' } ).then( function( data ) { return provider.getQueryPage( file, data ); @@ -90,9 +89,7 @@ !!( data['query-continue'] && data['query-continue'].globalusage ) ); } ); - } - - return this.cache[cacheKey]; + } ); }; mw.mmv.provider.GlobalUsage = GlobalUsage; diff --git a/resources/mmv/provider/mmv.provider.Image.js b/resources/mmv/provider/mmv.provider.Image.js index 541ba7b4b..d23a0e09e 100644 --- a/resources/mmv/provider/mmv.provider.Image.js +++ b/resources/mmv/provider/mmv.provider.Image.js @@ -61,7 +61,9 @@ } } - return this.cache[cacheKey]; + return this.cache[cacheKey].fail( function ( error ) { + mw.log( provider.constructor + ' provider failed to load: ', error ); + } ); }; /** diff --git a/resources/mmv/provider/mmv.provider.ImageInfo.js b/resources/mmv/provider/mmv.provider.ImageInfo.js index 478a8c173..f00e17f66 100644 --- a/resources/mmv/provider/mmv.provider.ImageInfo.js +++ b/resources/mmv/provider/mmv.provider.ImageInfo.js @@ -73,17 +73,16 @@ * @return {jQuery.Promise} a promise which resolves to an mw.mmv.model.Image object. */ ImageInfo.prototype.get = function( file ) { - var provider = this, - cacheKey = file.getPrefixedDb(); + var provider = this; - if ( !this.cache[cacheKey] ) { - this.cache[cacheKey] = this.api.get( { + return this.getCachedPromise( file.getPrefixedDb(), function () { + return provider.api.get( { action: 'query', prop: 'imageinfo', titles: file.getPrefixedDb(), - iiprop: this.iiprop, - iiextmetadatafilter: this.iiextmetadatafilter, - iiextmetadatalanguage: this.options.language, + iiprop: provider.iiprop, + iiextmetadatafilter: provider.iiextmetadatafilter, + iiextmetadatalanguage: provider.options.language, format: 'json' } ).then( function( data ) { return provider.getQueryPage( file, data ); @@ -96,9 +95,7 @@ return $.Deferred().reject( 'unknown error' ); } } ); - } - - return this.cache[cacheKey]; + } ); }; mw.mmv.provider.ImageInfo = ImageInfo; diff --git a/resources/mmv/provider/mmv.provider.ImageUsage.js b/resources/mmv/provider/mmv.provider.ImageUsage.js index ea36df3ff..4d8e8c128 100644 --- a/resources/mmv/provider/mmv.provider.ImageUsage.js +++ b/resources/mmv/provider/mmv.provider.ImageUsage.js @@ -47,16 +47,15 @@ * @return {jQuery.Promise} */ ImageUsage.prototype.get = function( file ) { - var provider = this, - cacheKey = file.getPrefixedDb(); + var provider = this; - if ( !this.cache[cacheKey] ) { - this.cache[cacheKey] = this.api.get( { + return this.getCachedPromise( file.getPrefixedDb(), function () { + return provider.api.get( { action: 'query', list: 'imageusage', iutitle: file.getPrefixedDb(), - iunamespace: this.options.namespaces, - iulimit: this.options.apiLimit, + iunamespace: provider.options.namespaces, + iulimit: provider.options.apiLimit, format: 'json' } ).then( function( data ) { return provider.getQueryField( 'imageusage', data ); @@ -76,9 +75,7 @@ !!( data['query-continue'] && data['query-continue'].imageusage ) ); } ); - } - - return this.cache[cacheKey]; + } ); }; mw.mmv.provider.ImageUsage = ImageUsage; diff --git a/resources/mmv/provider/mmv.provider.ThumbnailInfo.js b/resources/mmv/provider/mmv.provider.ThumbnailInfo.js index 0a0be6db3..ddff9c94c 100644 --- a/resources/mmv/provider/mmv.provider.ThumbnailInfo.js +++ b/resources/mmv/provider/mmv.provider.ThumbnailInfo.js @@ -45,8 +45,8 @@ var provider = this, cacheKey = file.getPrefixedDb() + '|' + ( width || '' ) + '|' + ( height || '' ); - if ( !this.cache[cacheKey] ) { - this.cache[cacheKey] = this.api.get( { + return this.getCachedPromise( cacheKey, function () { + return provider.api.get( { action: 'query', prop: 'imageinfo', titles: file.getPrefixedDb(), @@ -76,9 +76,7 @@ return $.Deferred().reject( 'unknown error' ); } } ); - } - - return this.cache[cacheKey]; + } ); }; mw.mmv.provider.ThumbnailInfo = ThumbnailInfo; diff --git a/resources/mmv/provider/mmv.provider.UserInfo.js b/resources/mmv/provider/mmv.provider.UserInfo.js index 05030444c..425601624 100644 --- a/resources/mmv/provider/mmv.provider.UserInfo.js +++ b/resources/mmv/provider/mmv.provider.UserInfo.js @@ -53,8 +53,8 @@ cacheKey = cacheKey + '|' + repoInfo.apiUrl; // local and remote user names could conflict } - if ( !this.cache[cacheKey] ) { - this.cache[cacheKey] = this.api.get( { + return this.getCachedPromise( cacheKey, function () { + return provider.api.get( { action: 'query', list: 'users', ususers: username, @@ -69,9 +69,7 @@ return new mw.mmv.model.User( username, mw.mmv.model.User.Gender.UNKNOWN ); } } ); - } - - return this.cache[cacheKey]; + } ); }; mw.mmv.provider.UserInfo = UserInfo;