Mailman 3 [Twisted-Python] IReactorTime.seconds: epoch time or no? - Twisted - python.org

newer
[Twisted-Python] Weekly Bug Summary

[Twisted-Python] IReactorTime.seconds: epoch time or no?

older
[Twisted-Python] Twisted 20.3.0...

Richard van der Hoff

23 Mar 2020 23 Mar '20

2:45 p.m.

HTTPFactory seems to think that `reactor.seconds` is reliably an epoch time (see https://github.com/twisted/twisted/blob/trunk/src/twisted/web/http.py#L3110, etc). On the other hand. AsyncioSelectorReactor.seconds() returns a monotonic time: python3 -c 'from twisted.internet import asyncioreactor; print(asyncioreactor.AsyncioSelectorReactor().seconds())' 41116.763594412 One of these is wrong... I think it's HTTPFactory making bad assumptions, but can anyone confirm the intention here? R

Reply

Sign in to reply online Use email software

Show replies by date

Amber Brown (hawkowl)

23 Mar 23 Mar

3:24 p.m.

On 24/3/20 6:45 am, Richard van der Hoff wrote:

HTTPFactory seems to think that `reactor.seconds` is reliably an epoch time (see https://github.com/twisted/twisted/blob/trunk/src/twisted/web/http.py#L3110, etc).

On the other hand. AsyncioSelectorReactor.seconds() returns a monotonic time:

python3 -c 'from twisted.internet import asyncioreactor; print(asyncioreactor.AsyncioSelectorReactor().seconds())' 41116.763594412

One of these is wrong... I think it's HTTPFactory making bad assumptions, but can anyone confirm the intention here?

It's a bug in asyncioreactor. IReactorTime defines it as "current time in seconds", which _probably_ should be defined nicer (https://twistedmatrix.com/documents/current/api/twisted.internet.interfaces....) but is generally epoch time. asyncioreactor does have a few problems with time: https://twistedmatrix.com/trac/ticket/9611 https://twistedmatrix.com/trac/ticket/9780 I can't find a bug for this issue, though, although I'm almost sure I've seen it before... so, if you could file one (and maybe a patch to fix it!) then that would be appreciated. - Amber

Reply

Sign in to reply online Use email software

Tom Most

24 Mar 24 Mar

6:05 p.m.

I'll offer a dissenting opinion: I've worked on systems where the reactor is patched to use monotonic time. (This is essential on embedded systems that lack a real-time clock.) I'm not aware of this causing any issues, and it fixed real problems. HTTPFactory is quite unusual in assuming that it is epoch time. Most parts of Twisted that require wall time call `time.time()` or equivalent directly. IMO HTTPFactory is in error here, and this is simply a bug. We should change HTTPFactory to use `time.time()` directly. Since the time base of IReactorTime.seconds() hasn't ever been clearly defined, I think that we should really go the other way: use `time.monotonic()` by default where possible. (Or, more realistically, make it an option and maybe the default.) ---Tom On Mon, Mar 23, 2020, at 1:24 PM, Amber Brown (hawkowl) wrote:

On 24/3/20 6:45 am, Richard van der Hoff wrote:

...
HTTPFactory seems to think that `reactor.seconds` is reliably an epoch time (see https://github.com/twisted/twisted/blob/trunk/src/twisted/web/http.py#L3110, etc).

On the other hand. AsyncioSelectorReactor.seconds() returns a monotonic time:

python3 -c 'from twisted.internet import asyncioreactor; print(asyncioreactor.AsyncioSelectorReactor().seconds())' 41116.763594412

One of these is wrong... I think it's HTTPFactory making bad assumptions, but can anyone confirm the intention here?

It's a bug in asyncioreactor. IReactorTime defines it as "current time in seconds", which _probably_ should be defined nicer (https://twistedmatrix.com/documents/current/api/twisted.internet.interfaces....) but is generally epoch time.

asyncioreactor does have a few problems with time:

https://twistedmatrix.com/trac/ticket/9611 https://twistedmatrix.com/trac/ticket/9780

I can't find a bug for this issue, though, although I'm almost sure I've seen it before... so, if you could file one (and maybe a patch to fix it!) then that would be appreciated.

- Amber

_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

Reply

Sign in to reply online Use email software

Glyph

25 Mar 25 Mar

2:02 a.m.

On Mar 24, 2020, at 4:05 PM, Tom Most wrote:

I'll offer a dissenting opinion:

I've worked on systems where the reactor is patched to use monotonic time. (This is essential on embedded systems that lack a real-time clock.) I'm not aware of this causing any issues, and it fixed real problems.

I am curious to know the problems that were fixed here, but, this is a violation of the IReactorTime contract, implicit as it may be. The reactor should have a monotonic-time interface, and maybe we should even use it to implement callLater. But .seconds() is wall-clock time.

HTTPFactory is quite unusual in assuming that it is epoch time. Most parts of Twisted that require wall time call `time.time()` or equivalent directly. IMO HTTPFactory is in error here, and this is simply a bug. We should change HTTPFactory to use `time.time()` directly.

Nope. This breaks a whole class of testing strategies, which are explicitly documented in https://twistedmatrix.com/documents/20.3.0/core/howto/trial.html#testing-sch... https://twistedmatrix.com/documents/20.3.0/core/howto/trial.html#testing-sch... . However, I can definitely see the need for monotonic time. `IReactorTimeEx` should probably include methods for getting monotonic time, getting civil times, scheduling callables at future civil times explicitly rather than using relative timestamps, etc. Possibly some of these should be different interfaces.

Since the time base of IReactorTime.seconds() hasn't ever been clearly defined, I think that we should really go the other way: use `time.monotonic()` by default where possible. (Or, more realistically, make it an option and maybe the default.)

Definitely not. Huge huge swathes of Twisted code would break, not least of which would be Twisted's own HTTP access logs which rely on .seconds() to be wall clock.

---Tom

On Mon, Mar 23, 2020, at 1:24 PM, Amber Brown (hawkowl) wrote:

...
On 24/3/20 6:45 am, Richard van der Hoff wrote:

...
HTTPFactory seems to think that `reactor.seconds` is reliably an epoch time (see https://github.com/twisted/twisted/blob/trunk/src/twisted/web/http.py#L3110, etc).

On the other hand. AsyncioSelectorReactor.seconds() returns a monotonic time:

python3 -c 'from twisted.internet import asyncioreactor; print(asyncioreactor.AsyncioSelectorReactor().seconds())' 41116.763594412

One of these is wrong... I think it's HTTPFactory making bad assumptions, but can anyone confirm the intention here?

It's a bug in asyncioreactor. IReactorTime defines it as "current time in seconds", which _probably_ should be defined nicer (https://twistedmatrix.com/documents/current/api/twisted.internet.interfaces....) but is generally epoch time.

asyncioreactor does have a few problems with time:

https://twistedmatrix.com/trac/ticket/9611 https://twistedmatrix.com/trac/ticket/9780

I can't find a bug for this issue, though, although I'm almost sure I've seen it before... so, if you could file one (and maybe a patch to fix it!) then that would be appreciated.

- Amber

_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

Reply

Sign in to reply online Use email software

Barry Scott

26 Mar 26 Mar

4:23 a.m.

On Wednesday, 25 March 2020 07:02:46 GMT Glyph wrote:

...
On Mar 24, 2020, at 4:05 PM, Tom Most wrote:

I'll offer a dissenting opinion:

I've worked on systems where the reactor is patched to use monotonic time. (This is essential on embedded systems that lack a real-time clock.) I'm not aware of this causing any issues, and it fixed real problems. I am curious to know the problems that were fixed here, but, this is a violation of the IReactorTime contract, implicit as it may be.

The reactor should have a monotonic-time interface, and maybe we should even use it to implement callLater. But .seconds() is wall-clock time.

...
HTTPFactory is quite unusual in assuming that it is epoch time. Most parts of Twisted that require wall time call `time.time()` or equivalent directly. IMO HTTPFactory is in error here, and this is simply a bug. We should change HTTPFactory to use `time.time()` directly. Nope. This breaks a whole class of testing strategies, which are explicitly documented in https://twistedmatrix.com/documents/20.3.0/core/howto/trial.html#testing-sc heduling <https://twistedmatrix.com/documents/20.3.0/core/howto/trial.html#testing-s cheduling> .

However, I can definitely see the need for monotonic time. `IReactorTimeEx` should probably include methods for getting monotonic time, getting civil times, scheduling callables at future civil times explicitly rather than using relative timestamps, etc. Possibly some of these should be different interfaces.

...
Since the time base of IReactorTime.seconds() hasn't ever been clearly defined, I think that we should really go the other way: use `time.monotonic()` by default where possible. (Or, more realistically, make it an option and maybe the default.) Definitely not. Huge huge swathes of Twisted code would break, not least of which would be Twisted's own HTTP access logs which rely on .seconds() to be wall clock.

Why does Twisted need to duplicate the built in python time features? What was wrong with using time.time() for the access logs? Surely reactors must be use monotonic time to avoid breaking protocol timing across wall-clock time being adjusted by ntp etc. Barry

...
---Tom

On Mon, Mar 23, 2020, at 1:24 PM, Amber Brown (hawkowl) wrote:

...
On 24/3/20 6:45 am, Richard van der Hoff wrote:

...
HTTPFactory seems to think that `reactor.seconds` is reliably an epoch time (see https://github.com/twisted/twisted/blob/trunk/src/twisted/web/http.py#L3 110, etc).

On the other hand. AsyncioSelectorReactor.seconds() returns a monotonic time:

python3 -c 'from twisted.internet import asyncioreactor; print(asyncioreactor.AsyncioSelectorReactor().seconds())' 41116.763594412

One of these is wrong... I think it's HTTPFactory making bad assumptions, but can anyone confirm the intention here?

It's a bug in asyncioreactor. IReactorTime defines it as "current time in seconds", which _probably_ should be defined nicer (https://twistedmatrix.com/documents/current/api/twisted.internet.interfa ces.IReactorTime.html#seconds) but is generally epoch time.

asyncioreactor does have a few problems with time:

https://twistedmatrix.com/trac/ticket/9611 https://twistedmatrix.com/trac/ticket/9780

I can't find a bug for this issue, though, although I'm almost sure I've seen it before... so, if you could file one (and maybe a patch to fix it!) then that would be appreciated.

- Amber

_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

Reply

Sign in to reply online Use email software

Richard van der Hoff

27 Mar 27 Mar

9:24 a.m.

On 26/03/2020 09:23, Barry Scott wrote:

Why does Twisted need to duplicate the built in python time features? > What was wrong with using time.time() for the access logs?

I feel like glyph's already answered this: use of `time` makes for poor testability. You might argue you don't care for access logs, since we aren't going to check the exact output, but (a) we probably *should* check the output matches the given timestamp, and (b) if a pattern's worth following, it's worth following everywhere.

Surely reactors must be use monotonic time to avoid breaking protocol timing across wall-clock time being adjusted by ntp etc.

Again, I think glyph's answered this. There *should* be a method for getting a monotonic-time, and things that do scheduling should use it; but that method shouldn't be `seconds()`. Thanks to Amber and glyph for answering my original question: I've opened https://twistedmatrix.com/trac/ticket/9787 to track the issue.

Reply

Sign in to reply online Use email software

1488

Age (days ago)

1492

Last active (days ago)

Download

5 comments

5 participants

tags

participants (5)

Amber Brown (hawkowl)
Barry Scott
Glyph
Richard van der Hoff
Tom Most