Debounce Timeout on XMLHTTPRequest if loading #330

pik · 2017-01-16T18:06:06Z

Addresses the same issue as #327 but keeps the timeout in case of the browser failing to correctly error out a stagnant request per @kegsay's comment.

kegsay · 2017-01-17T09:35:15Z

src/realtime-callbacks.js

+ *
+ * @param {function} func   callback to be called after a delay
+ * @param {Number} waitMs  number of milliseconds to delay by
+ *


Missing docs for jobName.

kegsay · 2017-01-17T09:36:25Z

src/realtime-callbacks.js

+
+module.exports.debounce = function(jobName, func, waitMs) {
+    if (jobName === undefined) {
+        while(_debouncers[jobName = Math.random().toString(16).slice(2, 10)]);


Why the while? This seems rather opaque given you're just generating an ID.

This is to make sure it doesn't collide with an existing random ID.

Aaah, right I see now. This line feels a bit too clever, you're:

Generating the random ID.

Assigning it to jobName.

Keying into _debouncers to check if there's a collision.

If there is a collision, rinse and repeat else continue to the next line.

...all on one line. Please can you break this up onto multiple lines which are more self-explanatory, and even add a comment to say what you're doing e.g:

// make sure the job name doesn't collide with an existing debouncer while (!jobName || _debouncers[jobName]) { jobName = Math.random().toString(16).slice(2, 10); }

kegsay · 2017-01-17T09:37:18Z

src/realtime-callbacks.js

+    return jobName;
+};
+/**
+ * Clear a debounce job..


kegsay · 2017-01-17T09:39:01Z

src/realtime-callbacks.js

+ *
+ * @param {String} a jobName to reference the debouncer.
+ *
+ * @return {Bool} whether a debouncer with jobName was cleared.


{boolean} for primitive booleans please. I don't think JSDoc3 will like {Bool}.

kegsay · 2017-01-17T09:40:55Z

src/realtime-callbacks.js

+
+    complete() {
+        delete _debouncers[this.jobName];
+        this.func();


You're assuming that a func is always provided, even though you're null guarding on start(func, waitMs). Which is it?

Supposing the first debounce call is like debounce('clientOffline', notifyClientOffline, 1000) this would allow future debounce calls to be made with just debounce('clientOffline') without having to maintain a reference to the same function (but also to over-ride the callback of the debounce if necessary).

Hmm, okay then.

kegsay

Broadly speaking this is looking good. Given the importance of getting this right (it affects literally every HTTP request) I would like to see some tests for this please.

kegsay · 2017-01-17T09:43:51Z

Tests are also failing on removeEventListener.

kegsay · 2017-01-20T10:28:02Z

spec/integ/matrix-client-syncing.spec.js

+            // Jasmine expect(<method>).toThrow() seems to break self-references in
+            // that method.
+            httpBackend.flush().done(function() {
+                done();


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

kegsay · 2017-01-20T10:29:42Z

spec/mock-request.js

-                    setTimeout(tryFlush, 0);
+                    // setImmediate(tryFlush);
+                    process.nextTick(tryFlush);
+                    // tick(0);


Presumably you're doing this to get around the fact that we're now mocking time so setTimeout isn't working as we expect. Can you add a comment to this effect please.

Oh, you do mention this further down! Wonderful! 😃

Though one wonders why setImmediate is used below and process.nextTick is used here? Any reason for it? If not, can we be consistent please.

kegsay · 2017-01-20T10:32:59Z

spec/mock-request.js

+     *
+     * @param {Number} waitMs number of miliseconds of timeout.
+     */
+    timeout: function(waitMs) {


What this actually does is advance the amount of time this HTTP request has taken - this may or may not result in a timeout. Can you please clarify this in the docs, else it appears as if you're saying "time out the request after waitMs".

kegsay · 2017-01-20T10:34:37Z

src/sync.js

@@ -869,6 +869,7 @@ SyncApi.prototype._processSyncResponse = function(syncToken, data) {
 * @return {promise}
 */
 SyncApi.prototype._startKeepAlives = function(delay) {
+    console.log('KEEP ALIVES');


Spurious console line.

pik · 2017-01-20T14:12:37Z

spec/mock-request.js

    this.requests = [];
    this.expectedRequests = [];
+    this.useMockClock = useMockClock ? true : false


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

pik · 2017-01-20T14:14:28Z

spec/mock-request.js

-                setTimeout(tryFlush, 5);
-                triedWaiting = true;
+            } else if (flushed === 0 && waitLoops < 50) {
+                waitLoops += 1


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...

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

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?

pik · 2017-01-20T14:15:49Z

src/http-api.js

-                        callbacks.clearTimeout(timeoutId);
+                    if (timeoutContext) {
+                        timeoutContext.stop();
+                        delete req.onprogres;


I'm not sure whether clearing onprogress on a request is required or not, once it has loaded or errored.

Well you're not spelling onprogress correctly...

pik · 2017-01-20T14:16:51Z

src/sync.js

@@ -36,7 +36,7 @@ const DEBUG = true;
 // beyond that and wedge forever, so we need to track how long we are willing
 // to keep open the connection. This constant is *ADDED* to the timeout= value
 // to determine the max time we're willing to wait.
-const BUFFER_PERIOD_MS = 80 * 1000;
+const BUFFER_PERIOD_MS = 30 * 1000;


Decreased this since it is now debounced; I think 60 seconds to wait for an onProgressEvent is enough before timing out?

Please keep this at 80. We can tweak it further should it become a problem.

kegsay · 2017-01-20T16:36:43Z

src/realtime-callbacks.js

+ */
+
+module.exports.debounce = function(jobName, func, waitMs) {
+    if (jobName === undefined || jobName === null) {


Given jobName is supposed to be a string, why not just:

if (!jobName) { // ... }

I guess jobName could be 0? If there is no reasonable case for that than ! should be fine.

kegsay · 2017-01-20T16:40:01Z

src/realtime-callbacks.js

+
+    start(func, waitMs) {
+        if (func) this.func = func;
+        if (!isNaN(waitMs)) this.waitMs = waitMs;


This won't do what you want it to do.

> isNaN(null) false

Meaning waitMs could be set to null. You really only want positive waitMs values, so just do:

if (waitMs > 0) { this.waitMs = waitMs; }

kegsay · 2017-01-20T16:43:25Z

spec/mock-request.js

@@ -1,14 +1,26 @@
 "use strict";
 const q = require("q");
+let clock = jasmine.Clock;


Why are you aliasing jasmine.Clock? It's useful to know at the callsites that you're manipulating jasmine's stuff and not our own, and the alias removes this context.

I was following the way it was done in realtime-callbacks.spec.js, /agree referencing it explicitly is better.

kegsay · 2017-01-20T16:47:19Z

src/http-api.js

@@ -679,6 +720,10 @@ module.exports.MatrixHttpApi.prototype = {
                    handlerFn(err, response, body);
                }
            );
+            if (timeoutContext) {
+                req.onprogress = timeoutContext.onProgressCheck;


How does this work as intended without .bind()ing, given you utilise this in onProgressCheck?

kegsay · 2017-01-20T16:54:14Z

src/http-api.js

+module.exports.__TimeoutContext = TimeoutContext;
+
+TimeoutContext.prototype.onProgressCheck = function(progressEvent) {
+    if (this.lastLoaded || progressEvent.loaded > this.lastLoaded) {


Why are you comparing Numbers to null initially? Why isn't lastLoaded set to 0? Also, you should comment why you are debouncing for any previous positive number. Isn't progressEvent.loaded > this.lastLoaded good enough? One can make the argument that if the request has wedged at say 20582 loaded bytes, and isn't changing, then you should NOT be debouncing and waiting forever if we are not making any progress.

Should be if (!this.lastLoaded || or rather if (this.lastLoaded != null. Is it safe to just set lastLoaded to 0, the first onProgressEvent may have loaded 0 or is that not the case?

kegsay · 2017-01-20T17:14:17Z

I think I'm about to make myself unpopular. 😞

You only need to do a few actions to debounce requests - repeatedly refresh a setTimeout, and yet this PR has got a lot of additional bloat around such a simple concept.

This PR is adding 2 new classes: Debouncer and TimeoutContext, both of which are internal to the SDK and both of which are used in precisely 1 place: to debounce HTTP requests. In order to expose enough information to drive this in tests you're exporting additional "private" functions like __TimeoutContext and __getDebouncers. You're modifying the HttpBackend to add additional timeout logic which is used solely in said tests. All of this adds what I feel is needless uncertainty to this PR.

I appreciate the desire to make generic, reusuable components which may be of use elsewhere, but the fact is that they aren't being used elsewhere at the moment, and I feel are adding needless complexity. I wouldn't be so paranoid if it weren't the fact that this is literally touching every single HTTP request that will be made in both clients and bridges: I want to have more confidence in this PR and I hoped the tests would do that, but it isn't.

Can you please re-write this as simply as possible: there is no shame to inlining the debouncing code in the _request function in HttpApi where appropriate if it improves clarity, which for such a simple task it really will. I appreciate you've spent a decent amount of time on this by now, and I'm sorry my bar is set quite so high, but I hope you can understand where I'm coming from given how important this section of code is.

EDIT: For an example of what I'm trying to guide you towards, is replacing this section in HttpApi._request:

if (localTimeoutMs) {
    timeoutId = callbacks.setTimeout(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
        }));
    }, localTimeoutMs);
}

with something to the effect of:

if (localTimeoutMs) {
    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
        }));
    };

    let timeoutId = setTimeout(timedOut, localTimeoutMs);
    let loaded = 0;

    req.onprogress = function(ev) {
        if (ev.loaded > loaded) {
            loaded = ev.loaded;
            clearTimeout(timeoutId);
            timeoutId = setTimeout(timedOut, localTimeoutMs);
        }
    }
}

pik · 2017-01-20T17:39:29Z

You only need to do a few actions to debounce requests - repeatedly

refresh a setTimeout, In order to repeatedly refresh a setTimeout two separate contexts are required, the `timeoutId` of the previous timeout to clear and the `loaded` number returned from the last ProgressEvent. Both of these need to be made available to the `onprogress` function and both need to be stored in a permanent data-structure since `loaded` and `timeoutId` will both change per `onprogress` call. If you would prefer I can inline the data-structure into the request creation (I think this is what you are suggesting?) but from my estimation that would add *more* rather than *reduce* the amount of untestable code. In fact currently the `/sync` function is not tested anywhere directly (it requires internal state and has no return value) --- all of the spec/integ tests use `startClient()`: hence the initially hackish setTimeout(tryFlush, 5) in the mock-request.

…

On Fri, Jan 20, 2017 at 2:14 PM, Kegsay ***@***.***> wrote: I think I'm about to make myself unpopular. 😞 You only need to do a few actions to debounce requests - repeatedly refresh a setTimeout, and yet this PR has got a *lot* of additional bloat around such a simple concept. This PR is adding 2 new classes: Debouncer and TimeoutContext, both of which are internal to the SDK and both of which are used in precisely 1 place: to debounce HTTP requests. In order to expose enough information to drive this in tests you're exporting additional "private" functions like __TimeoutContext and __getDebouncers. You're modifying the HttpBackend to add additional timeout logic which is used solely in said tests. All of this adds what I feel is needless uncertainty to this PR. I appreciate the desire to make generic, reusuable components which may be of use elsewhere, but the fact is that they *aren't* being used elsewhere at the moment, and I feel are adding needless complexity. I wouldn't be so paranoid if it weren't the fact that this is literally touching every single HTTP request that will be made in both clients and bridges: I want to have more confidence in this PR and I hoped the tests would do that, but it isn't. Can you please re-write this as simply as possible: there is no shame to inlining the debouncing code in the _request function in HttpApi where appropriate if it improves clarity, which for such a simple task it really will. I appreciate you've spent a decent amount of time on this by now, and I'm sorry my bar is set quite so high, but I hope you can understand where I'm coming from given how important this section of code is. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#330 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJwz-KtMN9QvA90mcmdXe1vGCguYaBnAks5rUOtqgaJpZM4Lk1z6> .

kegsay · 2017-01-20T17:50:41Z

from my estimation that would add more rather than reduce the amount of
untestable code

I don't care whether or not we test whether the loaded value is always increasing. I care about the underlying concept: if I do not respond to an HTTP request and do not call onprogress, do we time out after X seconds? If I do call onprogress, we should not time out after those same X seconds. That is the essence of what we are doing here, and seem testable to me.

pik · 2017-03-16T11:03:04Z

Closing since #392 is merged

pik force-pushed the debounce-sync-timeout branch from 3a7f1e7 to 5b4cea6 Compare January 16, 2017 22:36

kegsay reviewed Jan 17, 2017

View reviewed changes

src/realtime-callbacks.js

return jobName;

};

/**

* Clear a debounce job..

Copy link

Member

kegsay Jan 17, 2017

Choose a reason for hiding this comment

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

Extra .

kegsay reviewed Jan 17, 2017

View reviewed changes

kegsay requested changes Jan 17, 2017

View reviewed changes

pik force-pushed the debounce-sync-timeout branch from 5b4cea6 to e5a689a Compare January 17, 2017 14:49

kegsay reviewed Jan 20, 2017

View reviewed changes

pik force-pushed the debounce-sync-timeout branch 3 times, most recently from 3763439 to b6200c3 Compare January 20, 2017 14:08

pik commented Jan 20, 2017

View reviewed changes

kegsay reviewed Jan 20, 2017

View reviewed changes

pik force-pushed the debounce-sync-timeout branch from b80e207 to 2833021 Compare January 20, 2017 17:14

pik force-pushed the debounce-sync-timeout branch from ff5e5be to 73b6e95 Compare January 20, 2017 17:45

Debounce timedout error if progress is being made on an XMLHTTPRequest

f485519

pik force-pushed the debounce-sync-timeout branch 2 times, most recently from 900b68c to f485519 Compare January 20, 2017 18:02

pik closed this Mar 16, 2017

richvdh mentioned this pull request Mar 22, 2017

Remove default timeout on Sync #327

Closed

Debounce Timeout on XMLHTTPRequest if loading #330

Debounce Timeout on XMLHTTPRequest if loading #330

Conversation

pik commented Jan 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kegsay left a comment

Choose a reason for hiding this comment

kegsay commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pik Jan 20, 2017 • edited Loading

Choose a reason for hiding this comment

kegsay commented Jan 20, 2017 • edited Loading

pik commented Jan 20, 2017 via email

kegsay commented Jan 20, 2017

pik commented Mar 16, 2017

pik Jan 20, 2017 •

edited

Loading

kegsay commented Jan 20, 2017 •

edited

Loading