Fixed doPoll() spam calls at the start

[ric20007]

At the start of TradeOfferManager because of apiKey and the lastPoll default values, doPoll() was not being rate limited as it should.

[DoctorMcKay]

I'm not sure I understand what you've changed. Under the old code, the poll will be skipped if:

• apiKey is not set, OR
• _lastPoll is within 1000 ms of now

Under your proposed code, the poll will be skipped if:

• apiKey is not set, OR
• _lastPoll is within 1000 ms of now AND _lastPoll is not zero

However, 0 will never be within 1000 ms of now.

[andrewda]

Should be on same line as the closing curly brace.

[andrewda]

Super picky (sorry). This should be regular sentence capitalization, and the number sign is probably not needed:

"Seconds since last poll"

[andrewda]

Also super picky (sorry, again) but the "k" in "key" shouldn't be capitalized.

[ric20007]

I think the real problem is the _lastPool being 0, with the current code the doPoll() and _resetPollTimer() are executed many times as fast as the cpu can execute them (300, 500 or 1300 times on my testing runs).

They are stopped because the timer is being cleared and set again each time, but it seems inefficient to clear and set it again so many times.

The apiKey if branch currently also uses the _lastPool value when 0, that causes the weird looping timeouts, so 1000 miliseconds is used.
This if branch solves almost problems because usually apiKey is undefined when _lastPoll is 0, but it's best to cover all cases so I also checked if it's 0 on the other condition.

Regarding the emit debugs they can be deleted or sintax corrected, I forgot that they were something I added,

[Aareksio]

The problem is real, but I think @ric20007 doesn't understand it's root.
_resetPollTimer() takes time as the argument. Unless skipped, it'll use it to schedule the next check.

Here it's called with argument of const time = Date.now() - this._lastPoll, if time < 1000, which causes the problem if this._lastPoll and Date.now() are extremly close:

const now = Date.now();
const lastPoll = now - 1; // One ms ago
const timeSinceLastPoll = now - lastPoll;

setTimeout(doPoll, timeSinceLastPoll);

My suggestion is to save the minimum poll interval into constant, then call _resetPollTimer with minimumCallInterval - timeSinceLastPoll, if timeSinceLastPoll < minimumCallInterval, where timeSinceLastPoll is of course Date.now() - this._lastPoll.

See #237 for implementation.

[welwood08]

Why does _resetPollTimer() need to be called at all when apiKey is not set? It looks to me like setCookies already calls doPoll() after ensuring apiKey is set so any other call before that (from events) is unnecessary and could be an early return instead of setting the timer. Or am I missing something?

[ric20007]

@welwood08 I tried to keep the same logic as before while fixing the bug, in the comment above the _resetPollTimer call is said to reset the timer if we don't have the apiKey so i guess it's necessary.
I'm not entirely familiar with the inner workings of the _pollTimer and when it should be reset, cleared or left alone, i just wanted the bug to be fixed any other optimizations are welcomed.

[welwood08]

Right, but I think the bug is actually that polling shouldn't be attempted at all until apiKey is set. There's no point setting a timeout to run doPoll() again if apiKey is still going to be unset when it next runs (i.e. the comment you refer to is misleading).

The code that sets apiKey ensures it calls doPoll() unless a timer already exists, so in fact calling _resetPollTimer() when apiKey is unset can add a delay of up to pollInterval after apiKey is set before doPoll() runs properly.