From 9e89e71e0e8d445a31ea6c19b017497e1d26a390 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 30 Sep 2016 15:22:06 +0100 Subject: [PATCH] Fix leak of file upload objects After an upload completed, we were failing to delete the details of the upload from the list (because the comparison was bogus), meaning we leaked an object each time. While we're in the area: - make the request methods take an opts object (instead of a localTimeout param), and deprecate the WithPrefix versions. - make the non-xhr path for uploadContent use authedRequest instead of rolling its own. - make cancelUpload use the promise.abort() hack for simplicity --- lib/http-api.js | 191 +++++++++++++++++++++++++++++++----------------- 1 file changed, 122 insertions(+), 69 deletions(-) diff --git a/lib/http-api.js b/lib/http-api.js index 42bc54e30..bc5ac51ed 100644 --- a/lib/http-api.js +++ b/lib/http-api.js @@ -113,8 +113,6 @@ module.exports.MatrixHttpApi.prototype = { "Expected callback to be a function but got " + typeof callback ); } - var defer = q.defer(); - var url = this.opts.baseUrl + "/_matrix/media/v1/upload"; // browser-request doesn't support File objects because it deep-copies // the options using JSON.parse(JSON.stringify(options)). Instead of // loading the whole file into memory as a string and letting @@ -124,8 +122,9 @@ module.exports.MatrixHttpApi.prototype = { // of important here) var upload = { loaded: 0, total: 0 }; - + var promise; if (global.XMLHttpRequest) { + var defer = q.defer(); var xhr = new global.XMLHttpRequest(); upload.xhr = xhr; var cb = requestCallback(defer, callback, this.opts.onlyData); @@ -168,6 +167,7 @@ module.exports.MatrixHttpApi.prototype = { xhr.timeout_timer = callbacks.setTimeout(timeout_fn, 30000); defer.notify(ev); }); + var url = this.opts.baseUrl + "/_matrix/media/v1/upload"; url += "?access_token=" + encodeURIComponent(this.opts.accessToken); url += "&filename=" + encodeURIComponent(file.name); @@ -180,47 +180,48 @@ module.exports.MatrixHttpApi.prototype = { xhr.setRequestHeader("Content-Type", 'application/octet-stream'); } xhr.send(file); + promise = defer.promise; + + // dirty hack (as per _request) to allow the upload to be cancelled. + promise.abort = xhr.abort.bind(xhr); } else { var queryParams = { filename: file.name, - access_token: this.opts.accessToken }; - upload.request = this.opts.request({ - uri: url, - qs: queryParams, - method: "POST", - headers: {"Content-Type": file.type}, - body: file.stream - }, requestCallback(defer, callback, this.opts.onlyData)); + promise = this.authedRequest( + callback, "POST", "/upload", queryParams, file.stream, { + prefix: "/_matrix/media/v1", + localTimeoutMs: 30000, + headers: {"Content-Type": file.type}, + } + ); } - this.uploads.push(upload); - var self = this; - upload.promise = defer.promise.finally(function() { - var uploadsKeys = Object.keys(self.uploads); - for (var i = 0; i < uploadsKeys.length; ++i) { - if (self.uploads[uploadsKeys[i]].promise === defer.promise) { - self.uploads.splice(uploadsKeys[i], 1); + + // remove the upload from the list on completion + var promise0 = promise.finally(function() { + for (var i = 0; i < self.uploads.length; ++i) { + if (self.uploads[i] === upload) { + self.uploads.splice(i, 1); + return; } } }); - return upload.promise; + + // copy our dirty abort() method to the new promise + promise0.abort = promise.abort; + + upload.promise = promise0; + this.uploads.push(upload); + + return promise0; }, cancelUpload: function(promise) { - var uploadsKeys = Object.keys(this.uploads); - for (var i = 0; i < uploadsKeys.length; ++i) { - var upload = this.uploads[uploadsKeys[i]]; - if (upload.promise === promise) { - if (upload.xhr !== undefined) { - upload.xhr.abort(); - return true; - } else if (upload.request !== undefined) { - upload.request.abort(); - return true; - } - } + if (promise.abort) { + promise.abort(); + return true; } return false; }, @@ -271,11 +272,22 @@ module.exports.MatrixHttpApi.prototype = { * @param {string} method The HTTP method e.g. "GET". * @param {string} path The HTTP path after the supplied prefix e.g. * "/createRoom". - * @param {Object} queryParams A dict of query params (these will NOT be - * urlencoded). + * + * @param {Object=} queryParams A dict of query params (these will NOT be + * urlencoded). If unspecified, there will be no query params. + * * @param {Object} data The HTTP JSON body. - * @param {Number=} localTimeoutMs The maximum amount of time to wait before + * + * @param {Object=} opts additional options + * + * @param {Number=} opts.localTimeoutMs The maximum amount of time to wait before * timing out the request. If not specified, there is no timeout. + * + * @param {sting=} opts.prefix The full prefix to use e.g. + * "/_matrix/client/v2_alpha". If not specified, uses this.opts.prefix. + * + * @param {Object=} opts.headers map of additional request headers + * * @return {module:client.Promise} Resolves to {data: {Object}, * headers: {Object}, code: {Number}}. * If onlyData is set, this will resolve to the data @@ -283,18 +295,25 @@ module.exports.MatrixHttpApi.prototype = { * @return {module:http-api.MatrixError} Rejects with an error if a problem * occurred. This includes network problems and Matrix-specific error JSON. */ - authedRequest: function(callback, method, path, queryParams, data, localTimeoutMs) { - if (!queryParams) { queryParams = {}; } - queryParams.access_token = this.opts.accessToken; - var self = this; + authedRequest: function(callback, method, path, queryParams, data, opts) { + if (!queryParams) { + queryParams = {}; + } + if (!queryParams.access_token) { + queryParams.access_token = this.opts.accessToken; + } + var request_promise = this.request( - callback, method, path, queryParams, data, localTimeoutMs + callback, method, path, queryParams, data, opts ); + + var self = this; request_promise.catch(function(err) { if (err.errcode == 'M_UNKNOWN_TOKEN') { self.event_emitter.emit("Session.logged_out"); } }); + // return the original promise, otherwise tests break due to it having to // go around the event loop one more time to process the result of the request return request_promise; @@ -307,11 +326,22 @@ module.exports.MatrixHttpApi.prototype = { * @param {string} method The HTTP method e.g. "GET". * @param {string} path The HTTP path after the supplied prefix e.g. * "/createRoom". - * @param {Object} queryParams A dict of query params (these will NOT be - * urlencoded). + * + * @param {Object=} queryParams A dict of query params (these will NOT be + * urlencoded). If unspecified, there will be no query params. + * * @param {Object} data The HTTP JSON body. - * @param {Number=} localTimeoutMs The maximum amount of time to wait before + * + * @param {Object=} opts additional options + * + * @param {Number=} opts.localTimeoutMs The maximum amount of time to wait before * timing out the request. If not specified, there is no timeout. + * + * @param {sting=} opts.prefix The full prefix to use e.g. + * "/_matrix/client/v2_alpha". If not specified, uses this.opts.prefix. + * + * @param {Object=} opts.headers map of additional request headers + * * @return {module:client.Promise} Resolves to {data: {Object}, * headers: {Object}, code: {Number}}. * If onlyData is set, this will resolve to the data @@ -319,9 +349,13 @@ module.exports.MatrixHttpApi.prototype = { * @return {module:http-api.MatrixError} Rejects with an error if a problem * occurred. This includes network problems and Matrix-specific error JSON. */ - request: function(callback, method, path, queryParams, data, localTimeoutMs) { - return this.requestWithPrefix( - callback, method, path, queryParams, data, this.opts.prefix, localTimeoutMs + request: function(callback, method, path, queryParams, data, opts) { + opts = opts || {}; + var prefix = opts.prefix || this.opts.prefix; + var fullUri = this.opts.baseUrl + prefix + path; + + return this.requestOtherUrl( + callback, method, fullUri, queryParams, data, opts ); }, @@ -347,16 +381,16 @@ module.exports.MatrixHttpApi.prototype = { * object only. * @return {module:http-api.MatrixError} Rejects with an error if a problem * occurred. This includes network problems and Matrix-specific error JSON. + * + * @deprecated prefer authedRequest with opts.prefix */ authedRequestWithPrefix: function(callback, method, path, queryParams, data, prefix, localTimeoutMs) { - var fullUri = this.opts.baseUrl + prefix + path; - if (!queryParams) { - queryParams = {}; - } - queryParams.access_token = this.opts.accessToken; - return this._request( - callback, method, fullUri, queryParams, data, localTimeoutMs + return this.authedRequest( + callback, method, path, queryParams, data, { + localTimeoutMs: localTimeoutMs, + prefix: prefix, + } ); }, @@ -382,15 +416,16 @@ module.exports.MatrixHttpApi.prototype = { * object only. * @return {module:http-api.MatrixError} Rejects with an error if a problem * occurred. This includes network problems and Matrix-specific error JSON. + * + * @deprecated prefer request with opts.prefix */ requestWithPrefix: function(callback, method, path, queryParams, data, prefix, localTimeoutMs) { - var fullUri = this.opts.baseUrl + prefix + path; - if (!queryParams) { - queryParams = {}; - } - return this._request( - callback, method, fullUri, queryParams, data, localTimeoutMs + return this.request( + callback, method, path, queryParams, data, { + localTimeoutMs: localTimeoutMs, + prefix: prefix, + } ); }, @@ -400,11 +435,22 @@ module.exports.MatrixHttpApi.prototype = { * success/failure. See the promise return values for more information. * @param {string} method The HTTP method e.g. "GET". * @param {string} uri The HTTP URI - * @param {Object} queryParams A dict of query params (these will NOT be - * urlencoded). + * + * @param {Object=} queryParams A dict of query params (these will NOT be + * urlencoded). If unspecified, there will be no query params. + * * @param {Object} data The HTTP JSON body. - * @param {Number=} localTimeoutMs The maximum amount of time to wait before + * + * @param {Object=} opts additional options + * + * @param {Number=} opts.localTimeoutMs The maximum amount of time to wait before * timing out the request. If not specified, there is no timeout. + * + * @param {sting=} opts.prefix The full prefix to use e.g. + * "/_matrix/client/v2_alpha". If not specified, uses this.opts.prefix. + * + * @param {Object=} opts.headers map of additional request headers + * * @return {module:client.Promise} Resolves to {data: {Object}, * headers: {Object}, code: {Number}}. * If onlyData is set, this will resolve to the data @@ -413,12 +459,18 @@ module.exports.MatrixHttpApi.prototype = { * occurred. This includes network problems and Matrix-specific error JSON. */ requestOtherUrl: function(callback, method, uri, queryParams, data, - localTimeoutMs) { - if (!queryParams) { - queryParams = {}; + opts) { + if (opts === undefined || opts === null) { + opts = {}; + } else if (isFinite(opts)) { + // opts used to be localTimeoutMs + opts = { + localTimeoutMs: opts + }; } + return this._request( - callback, method, uri, queryParams, data, localTimeoutMs + callback, method, uri, queryParams, data, opts ); }, @@ -441,16 +493,15 @@ module.exports.MatrixHttpApi.prototype = { return this.opts.baseUrl + prefix + path + queryString; }, - _request: function(callback, method, uri, queryParams, data, localTimeoutMs) { + _request: function(callback, method, uri, queryParams, data, opts) { if (callback !== undefined && !utils.isFunction(callback)) { throw Error( "Expected callback to be a function but got " + typeof callback ); } + opts = opts || {}; + var self = this; - if (!queryParams) { - queryParams = {}; - } if (this.opts.extraParams) { for (var key in this.opts.extraParams) { if (!this.opts.extraParams.hasOwnProperty(key)) { continue; } @@ -462,6 +513,7 @@ module.exports.MatrixHttpApi.prototype = { var timeoutId; var timedOut = false; var req; + var localTimeoutMs = opts.localTimeoutMs; if (localTimeoutMs) { timeoutId = callbacks.setTimeout(function() { timedOut = true; @@ -488,6 +540,7 @@ module.exports.MatrixHttpApi.prototype = { body: data, json: true, timeout: localTimeoutMs, + headers: opts.headers || {}, _matrix_opts: this.opts }, function(err, response, body) {