Use a monotonic clock for WebsocketSession's session_time

[outputenable]

What this PR solves

Automatically scheduled actions like ping etc. and various other timeouts rely on WebsocketSession's session_time property to determine when to execute/fire. This property is in turn based on Python's
time.time() which does not guarantee to always return non-decreasing values though. To quote the module documentation,

"[...] it can return a lower value than a previous call if the system clock has been set back between the two calls." Source: https://docs.python.org/2/library/time.html#time.time

In order to not fall victim to such quirks it is common for timeouts and stuff like that to use a so-called monotonic clock which---as their name implies---cannot go backwards due to e.g. system clock updates. Accordingly Python 3 also introduced such a clock in release 3.3, see:

• https://docs.python.org/3/library/time.html#time.monotonic

A rationale can be found in the associated "PEP 418 -- Add monotonic time, performance counter, and process time functions":

• https://www.python.org/dev/peps/pep-0418/

How it is done

There is an (actively maintained, by the looks of it) compatibility library available on PyPI fittingly called monotonic which provides a suitable monotonic clock for both Python 2 and 3:

• https://pypi.org/project/monotonic/

This PR suggests to replace the time.time() calls to initialize and update the session time with calls to monotonic.monotonic() which is not affected by changes of the system time. This of course introduces a new runtime dependency on the monotonic module. If import monotonic fails we can simply fall back on time.time() for backwards compatibility.

What to look out for

Event.receive_time is still using time.time() as before. (Note that the reference point of the values returned by a monotonic clock is generally undefined.)

Review

• Ready for review

[outputenable]

Hm, I see that the tests failed on CircleCI, but I'm afraid there's nothing I can do about it. It seems that the tox step fails because it can't report coverage results back to Coveralls due to a missing "repo token":

CoverallsException: Not on Travis or CircleCI. You have to provide either repo_token in .coveralls.yml or set the COVERALLS_REPO_TOKEN env var.

Now CircleCI also says (near the beginning of the build) that it is

Suppressing export of environment variables COVERALLS_REPO_TOKEN fork [sic] PR builds

so I guess that this is how it's supposed to be...?

[willmcgugan]

First PR? Congrats.

Good call, but I'd make a couple of changes. Also could you add monotonic to requirements?

[willmcgugan]

Best to explicitly catch an ImportError in the off chance the module has any import time side-effects.

[willmcgugan]

If its imported this way, it's not strictly monotonic time. Suggest calling it get_time (for both).

[willmcgugan]

@outputenable Think there is a misconfiguration our side. Seems to work with PRs from within our organisation. Will look in to that.

[outputenable]

Agreed, I've made the code changes you suggested, looks better now. Could you expand a little on the requirements thing though? Should I add monotonic in setup.py, or elsewhere? (Oh and thank you for making such a nice library available as open source!)

[willmcgugan]

Glad you find it useful!

If you could add monotonic==1.5 to setup.py (install_requires), that will ensure monotonic is installed alongside Lomond. On reflection, you won't need to catch that ImportError, as it should always be installed.

I may actually end up copying monotonic.py in to this repos, if the licenses are compatible. But I'll leave that for another update.

Thanks.

[outputenable]

Ah OK, I thought monotonic would be optional and wasn't sure where/how to add such a requirement. So I've

• added install_requires monotonic>=1.5 (>= instead of == as that's also used for the six dependency);
• removed the try/except around the import;
• renamed get_time to monotonic_time again, which IMO is appropriate now and avoids confusion.

What do you think?

[willmcgugan]

Hi @outputenable I figured out the circleci issue. The env var is only available in our organisation. Could you please add ignore_outcome=true to tox.ini, under the [testenv:coverage] section? That will let us merge this.

[outputenable]

Uh-oh, maybe I shouldn't have clicked that 'Merge' button so quickly...!? *sigh*

Anyway, the Coveralls issue seems to be solved, but CircleCI hit a different problem now which to me looks like a genuine test failure of test_unresponsive. I'm not familiar with tox or freezegun, but that test apparently relies on a fake system time and freeze_time() doesn't affect monotonic.monotonic(). There's a freezegun pull request to support Python 3's time.monotonic(), but that's been dormant for quite a while and probably wouldn't help us anyway. I'm afraid this is a little over my head still (Python is not my primary language) and I'll have to look into it some more in the next couple days.

[willmcgugan]

I'll take a look at this soon. Suspect we can rework the tests to not require freezegun...