Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Debounce Timeout on XMLHTTPRequest if loading #330

Closed
wants to merge 1 commit into from
Closed

Debounce Timeout on XMLHTTPRequest if loading #330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions spec/integ/matrix-client-syncing.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ describe("MatrixClient syncing", function() {

beforeEach(function() {
utils.beforeEach(this); // eslint-disable-line no-invalid-this
httpBackend = new HttpBackend();
httpBackend = new HttpBackend(true);
sdk.request(httpBackend.requestFn);
client = sdk.createClient({
baseUrl: baseUrl,
Expand All @@ -43,12 +43,25 @@ describe("MatrixClient syncing", function() {
presence: {},
};

it("should start keepalives if a /sync hits a timeout error", function(done) {
httpBackend.when("GET", "/sync").respond(200, syncData).waitFor((30+80+1)*1000);
client.startClient();
spyOn(client._syncApi, '_startKeepAlives').andCallThrough();
// Work-around since
// Jasmine expect(<method>).toThrow() seems to break self-references in
// that method.
httpBackend.flush().done(function() {
expect(client._syncApi._startKeepAlives).toHaveBeenCalled();
done();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing assertion on _startKeepAlives? To check if it has/has not been called?

});
});

it("should /sync after /pushrules and /filter.", function(done) {
httpBackend.when("GET", "/sync").respond(200, syncData);

client.startClient();

spyOn(client._syncApi, '_startKeepAlives').andCallThrough();
httpBackend.flush().done(function() {
expect(client._syncApi._startKeepAlives).not.toHaveBeenCalled();
done();
});
});
Expand All @@ -59,7 +72,6 @@ describe("MatrixClient syncing", function() {
httpBackend.when("GET", "/sync").check(function(req) {
expect(req.queryParams.since).toEqual(syncData.next_batch);
}).respond(200, syncData);

client.startClient();

httpBackend.flush().done(function() {
Expand All @@ -76,7 +88,6 @@ describe("MatrixClient syncing", function() {
},
rooms: {
join: {

},
},
};
Expand Down
64 changes: 52 additions & 12 deletions spec/mock-request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,25 @@
"use strict";
const q = require("q");
let fakeDate;
let callbacks = require("../lib/realtime-callbacks");

let tick = function tick(millis) {
// make sure we tick the fakedate first, otherwise nothing will happen!
fakeDate += millis;
jasmine.Clock.tick(millis);
};


/**
* Construct a mock HTTP backend, heavily inspired by Angular.js.
* @constructor
*/
function HttpBackend() {
function HttpBackend(useMockClock) {
this.requests = [];
this.expectedRequests = [];
this.useMockClock = useMockClock ? true : false
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally mock-requests should always use a mock-clock but that seems to be break some internal setTimeout uses?

const self = this;

// the request function dependency that the SDK needs.
this.requestFn = function(opts, callback) {
const req = new Request(opts, callback);
Expand Down Expand Up @@ -41,11 +52,20 @@ HttpBackend.prototype = {
const defer = q.defer();
const self = this;
let flushed = 0;
let triedWaiting = false;
let waitLoops = 0;
console.log(
"HTTP backend flushing... (path=%s numToFlush=%s)", path, numToFlush
);
const tryFlush = function() {
// Mock out Clock for .timeout() tests
if (this.useMockClock) {
jasmine.Clock.useMock();
fakeDate = Date.now();
callbacks.setNow(function() {
return fakeDate;
});
}

const tryFlush = function tryFlush() {
// if there's more real requests and more expected requests, flush 'em.
console.log(
" trying to flush queue => reqs=%s expected=%s [%s]",
Expand All @@ -57,23 +77,27 @@ HttpBackend.prototype = {
flushed += 1;
if (numToFlush && flushed === numToFlush) {
console.log(" [%s] Flushed assigned amount: %s", path, numToFlush);
// Restore Clock
if (self.useMockClock) {
callbacks.setNow();
}
defer.resolve();
} else {
setTimeout(tryFlush, 0);
setImmediate(tryFlush);
}
} else if (flushed === 0 && !triedWaiting) {
// we may not have made the request yet, wait a generous amount of
// time before giving up.
setTimeout(tryFlush, 5);
triedWaiting = true;
} else if (flushed === 0 && waitLoops < 50) {
waitLoops += 1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setTimeout for waiting isn't compatible with mocking that out, so I ended up with something like this -- but it seems like a hack. I believe Jasmine has a waitFor (but it's removed in 2.0) it would be great to do this in a nicer way though...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You absolutely need to comment this and explain why you're doing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like very much to improve this rather than committing it to the code-base (haven't done a lot of testing in Node), are you aware of a better strategy or a reference code-base to take a peek at?

setImmediate(tryFlush);
} else {
console.log(" no more flushes. [%s]", path);
// Restore Clock
if (self.useMockClock) {
callbacks.setNow();
}
defer.resolve();
}
};

setTimeout(tryFlush, 0);

setImmediate(tryFlush);
return defer.promise;
},

Expand Down Expand Up @@ -116,6 +140,9 @@ HttpBackend.prototype = {
matchingReq.checks[j](req);
}
testResponse = matchingReq.response;
if (matchingReq._waitMs) {
tick(matchingReq._waitMs);
}
console.log(" responding to %s", matchingReq.path);
let body = testResponse.body;
if (Object.prototype.toString.call(body) == "[object Function]") {
Expand Down Expand Up @@ -203,6 +230,7 @@ ExpectedRequest.prototype = {
* @param {Number} code The HTTP status code.
* @param {Object|Function} data The HTTP JSON body. If this is a function,
* it will be invoked when the JSON body is required (which should be returned).
* @return {Request} for chaining calls.
*/
respond: function(code, data) {
this.response = {
Expand All @@ -213,6 +241,7 @@ ExpectedRequest.prototype = {
body: data,
err: null,
};
return this;
},

/**
Expand All @@ -230,6 +259,17 @@ ExpectedRequest.prototype = {
err: err,
};
},

/**
* Advance the time before processing a request by waitMs.
*
* @param {Number} waitMs number of miliseconds of timeout.
* @return {Request} for chaining calls.
*/
waitFor: function(waitMs) {
this._waitMs = waitMs;
return this;
},
};

/**
Expand Down
26 changes: 21 additions & 5 deletions src/http-api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,22 +627,35 @@ module.exports.MatrixHttpApi.prototype = {

const defer = q.defer();

let timeoutId;
let timedOut = false;
let req;
let loaded;
let timeoutId;
let onProgressFunc;
const localTimeoutMs = opts.localTimeoutMs || this.opts.localTimeoutMs;

if (localTimeoutMs) {
timeoutId = callbacks.setTimeout(function() {
let timedOut = function () {
timedOut = true;
if (req && req.abort) {
req.abort();
}
defer.reject(new module.exports.MatrixError({
error: "Locally timed out waiting for a response",
errcode: "ORG.MATRIX.JSSDK_TIMEOUT",
timeout: localTimeoutMs,
timeout: localTimeoutMs
}));
}, localTimeoutMs);
};

timeoutId = setTimeout(timedOut, localTimeoutMs);

onProgressFunc = function(ev) {
if (loaded === null || ev.loaded > loaded) {
loaded = ev.loaded;
clearTimeout(timeoutId);
timeoutId = setTimeout(timedOut, localTimeoutMs);
}
};
}

const reqPromise = defer.promise;
Expand All @@ -662,7 +675,7 @@ module.exports.MatrixHttpApi.prototype = {
},
function(err, response, body) {
if (localTimeoutMs) {
callbacks.clearTimeout(timeoutId);
clearTimeout(timeoutId);
if (timedOut) {
return; // already rejected promise
}
Expand All @@ -684,6 +697,9 @@ module.exports.MatrixHttpApi.prototype = {
// abort() operations on underlying HTTP requests :(
reqPromise.abort = req.abort.bind(req);
}
if (localTimeoutMs) {
req.onprogress = onProgressFunc;
}
} catch (ex) {
defer.reject(ex);
if (callback) {
Expand Down