Opened 16 years ago

Closed 15 years ago

#1938 defect closed fixed (fixed)

web.Session.checkExpired and web.Site.makeSession do not keep DelayedCall handles

Reported by: acapnotic Owned by: acapnotic
Priority: highest Milestone:
Component: web Keywords: web session delayedcall
Cc: Branch:
Author:

Description

web.Session.checkExpired and web.Site.makeSession do not keep DelayedCall handles, which makes Trial unhappy.

Attachments (1)

session-delayedcall-1938.diff (2.8 KB) - added by acapnotic 16 years ago.
patch against rev 17599

Download all attachments as: .zip

Change History (8)

comment:1 Changed 16 years ago by Jean-Paul Calderone

Priority: normalhighest

It will be nice to have this fixed. Leaving crap dangling in the reactor sucks and makes writing tests hard. A few points:

  • Having the LoopingCall as an instance attribute creates a cycle. It should be explicitly broken when the timeout expires.
  • self.checkExpiredLoop.call.reset(self.checkExpiredLoop.interval) needs to be different somehow. The call attribute is not public. Arguably the running attribute accessed on the previous line is also not public, but if you document it on LoopingCall I guess it's okay.
  • The test method should be named with an underscore instead of camel-case.
  • The test method should use a fake timing implementation (see twisted.internet.task.Clock) instead of relying on trial's dirty reactor warning.

comment:2 Changed 16 years ago by acapnotic

well, the easy way to fix the lines that refer to questionably-private LoopingCall attributes is to just delete them, as they're premature optimization anyhow.

I agree that the relying on trial's dirty reactor warning is pretty dirty, but

1) how to integrate Clock here is non-obvious 2) Clock doesn't actually address the question of "are there outstanding delayedCalls after expire() has been called," which is what I was actually wanting to test. (Other Session tests wouldn't hurt, but they're not tied to this ticket.)

Changed 16 years ago by acapnotic

patch against rev 17599

comment:3 Changed 16 years ago by Stephen Thorne

Thankyou for the test. I find tests that don't contain assertions a little funky. Perhaps you could assert that the looping call is not None after the touch, and is None after the expire?

Please add a docstrings to every method that you've modified that doesn't already have a docstring.

Please make server.py pyflakes as clean as possible. There are expected pyflakes failures, but also oddities. Why does t.w.server imports things like 'server' and then doesn't use it.

For convenience, I've made a branch and put the patch into it, see session-delayedcall-1938.

comment:4 Changed 15 years ago by acapnotic

Keywords: review added

the test now explicitly checks reactor.getDelayedCalls rather than leaving it up to trial internals,

docstring added to Session.checkExpired,

politely declining the opportunity to do maintenance on t.web.server that is not related to this ticket.

comment:5 Changed 15 years ago by radix

Owner: changed from acapnotic to radix

comment:6 Changed 15 years ago by radix

Keywords: review removed
Owner: changed from radix to acapnotic

+1, please merge.

As for twisted.internet.task.Clock: given that, indeed, there is no obvious way to integrate it (in *any* code, let alone in this case), and that this test doesn't actually involve waiting for real time to pass, it's OK to go in.

Another battle won in the war for testability.

comment:7 Changed 15 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

This branch was merged some time ago in r17815 which incorrectly reported itself as fixing #1638

Note: See TracTickets for help on using tickets.