matrix/matrix-js-sdk
1
0
Fork 0
mirror of https://github.com/matrix-org/matrix-js-sdk.git synced 2025-08-09 10:22:46 +03:00
Code Activity

Fix leak of file upload objects

Browse Source 
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
This commit is contained in:
Richard van der Hoff
2016-09-30 15:22:06 +01:00
parent b784d1a5e7
commit 9e89e71e0e
1 changed files with 122 additions and 69 deletions
Download Patch File Download Diff File Expand all files Collapse all files

191
lib/http-api.js
View File

@@ -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 <b>after</b> 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 <code>{data: {Object},
* headers: {Object}, code: {Number}}</code>.
* If <code>onlyData</code> is set, this will resolve to the <code>data</code>
@@ -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 <b>after</b> 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 <code>{data: {Object},
* headers: {Object}, code: {Number}}</code>.
* If <code>onlyData</code> is set, this will resolve to the <code>data</code>
@@ -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 <code>{data: {Object},
* headers: {Object}, code: {Number}}</code>.
* If <code>onlyData</code> is set, this will resolve to the <code>data</code>
@@ -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) {