Skip to content

Commit

Permalink
Fix leak of file upload objects
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@richvdh
richvdh committed Sep 30, 2016
1 parent b784d1a commit 9e89e71
Showing 1 changed file with 122 additions and 69 deletions.
191 changes: 122 additions & 69 deletions lib/http-api.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
Expand Down Expand Up @@ -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);

Expand All @@ -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;
},
Expand Down Expand Up @@ -271,30 +272,48 @@ 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>
* object only.
* @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;
Expand All @@ -307,21 +326,36 @@ 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>
* object only.
* @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
);
},

Expand All @@ -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,
}
);
},

Expand All @@ -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,
}
);
},

Expand All @@ -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>
Expand All @@ -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
);
},

Expand All @@ -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; }
Expand All @@ -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;
Expand All @@ -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) {
Expand Down

0 comments on commit 9e89e71

Please sign in to comment.