FilesystemLock needs a method that defers until it can acquire a lock

[wsanchez]

FilesystemLock? has a lock() method that returns True or False, which is swell, but sometimes you want to wait until you can aquired lock, in a Twisted-friendly way.

[wsanchez]

Branch: lockfile-deferred-2180

[exarkun]

Straightforward stuff:

• tests fail in the branch
• new tests don't have docstrings

Other stuff:

• I'm not sure interval should be exposed. It is not inconceivable that this feature could be provided without polling in the future, and before that we may even want to use a backoff technique or do some other kind of scheduling.
• Oops, big problem. twisted.python isn't allowed to import anything from twisted.internet. I suppose that's why there was no Deferred support here before. Not really sure what to do about this. Maybe this functionality should be offered via a subclass somewhere in t.i?

[exarkun]

Any plans to finish this?

[wsanchez]

Well, I don't really know how we want to work around the twisted.python / twisted.internet dependancy.

[exarkun]

Two possibilities come to mind:

• parameterize the scheduler so FilesystemLock? can schedule tasks but doesn't know about the reactor.
• Put a subclass in t.internet somewhere and add the feature to it.

[therve]

Comments:

• revert changes in lockfile and test_lockfile as it doesn't seem to be real changes
• IReactorTime is not necessary on Clock: just assign your scheduler method to reactor.callLater, and to clock.callLater in the tests
• test methods should be named test_waitFoo instead of testWaitFoo
• several typos in DeferredFilesystemLock and tests docstrings: aquired

Otherwise, good work with great tests and docs.

[dreid]

Reverted the lockfile and test_lockfile Fixed typos, renamed test methods.

Scheduler is parameterized for reasons other than just the unittests. In the future it'll allow us to employ a non polling mechanism for checking for the lock. Adapting to IReactorTime also provides a more useful error should someone pass in an object that doesn't implement it.

Thanks for the review.

[therve]

I still get a modification on test_lockfile... Nothing harmful.

The new naming on test methods (is there a doc somewhere?) is test_lowercaseName. Sorry for not being clear.

OK for IReactorTime, I just wanted to be sure you wanted to do that :).

Then please merge.

[dreid]

(In [19626]) Merge lockfile-deferred-2180

Author: dreid, wsanchez Reviewer: therve, exarkun Fixes #2180

Adds a DeferredFilesystemLock? which has a deferUntilLocked method that returns a deferred that will fire when the lock is successfully aquired or fail after a timeout is reached.

[exarkun]

(In [19627]) Revert r19626 - new API incorrectly documented

Refs #2180

DeferredFilesystemLock?.deferUntilLocked is documented as taking an interval parameter, but it does not.

[therve]

It's a pity you removed the documentation of interval completely...

[exarkun]

Awkward phrasing:

Test the behavior of the DeferredFilesystemLock

Drop the 2nd the, I think. Also this might benefit from L{} .

All the tests in DeferredFilesystemLockTestCase? start by creating a clock and making a temporary directory. Perhaps setUp would be in order?

Two lines in DeferredFilesystemLock? have no test coverage:

    3:         if not scheduler:
>>>>>>             from twisted.internet import reactor
>>>>>>             scheduler = reactor

I can think of some possible fixes:

• make scheduler a required argument
• operate in the absence of a scheduler (don't support timeouts)
• write a test which doesn't pass in a scheduler and just asserts something about the scheduler attribute after instantiation (not extremely cool)

Also, the doc for scheduler in __init__ says it should be an IReactorTime (L{} again ;) provider, but then the implementation adapts the object to IReactorTime, so while the docstring is correct, strictly speaking, the code will also work if any object adaptable to IReactorTime is passed in. This will be a bigger deal when IReactorFilesystem or whatever is added, but it could probably be improved now.

I think the check inside tryNow inside deferUntilLocked to see if time has expired is slightly off. Consider the case where a function blocks the reactor for a significant amount of time (or a less significant amount of time, but the lock operation is given a large timeout). The requested timeout may expired before c is incremented the necessary number of times. The code probably needs to actually check the time while scheduling itself and deciding whether or not to time out. Of course, another possibility is to document the timeout as being very low resolution.

Is there a reason test_waitUntilLockedWithTimeoutUnlocked defines onTimeout instead of using assertFailure?

[dreid]

Replying to exarkun:

Awkward phrasing: {{{ Test the behavior of the DeferredFilesystemLock? }}} Drop the 2nd the, I think. Also this might benefit from L{} . All the tests in DeferredFilesystemLockTestCase? start by creating a clock and making a temporary directory. Perhaps setUp would be in order?

Ok.

Two lines in DeferredFilesystemLock? have no test coverage: {{{ 3: if not scheduler:

from twisted.internet import reactor scheduler = reactor

}}} I can think of some possible fixes: * make scheduler a required argument

That sounds like a hassle for the user. Unless I still allow None to be passed in defaulting to the reactor. Otherwise the user has to import the reactor every time they want to use the DFL in normal circumstances.

* operate in the absence of a scheduler (don't support timeouts)

Don't support timeouts, and don't actually defer, sounds more like what this option would get you. Atleast given the current implementation where we only support IReactorTime based schedulers. In the future when we have a real non-polling mechanism for waiting till we aquire the lock this might be a more viable option. But right now it sounds pretty bogus. In the abscence of the scheduler I'd basically just be doing.

if self.lock():
  return defer.succeed(None)
else:
  return defer.fail(None)

* write a test which doesn't pass in a scheduler and just asserts something about the scheduler attribute after instantiation (not extremely cool)

Also, the doc for scheduler in __init__ says it should be an IReactorTime (L{} again ;) provider, but then the implementation adapts the object to IReactorTime, so while the docstring is correct, strictly speaking, the code will also work if any object adaptable to IReactorTime is passed in. This will be a bigger deal when IReactorFilesystem or whatever is added, but it could probably be improved now.

I think "An object adaptable to IReactorTime" might be a more correct docstring. I don't see a reason why "adaptable" isn't what we want, nor do I understand the problem with IReactorFilesystem.

I think the check inside tryNow inside deferUntilLocked to see if time has expired is slightly off. Consider the case where a function blocks the reactor for a significant amount of time (or a less significant amount of time, but the lock operation is given a large timeout). The requested timeout may expired before c is incremented the necessary number of times. The code probably needs to actually check the time while scheduling itself and deciding whether or not to time out. Of course, another possibility is to document the timeout as being very low resolution.

I'll fix this.

Is there a reason test_waitUntilLockedWithTimeoutUnlocked defines onTimeout instead of using assertFailure?

Because that test isn't asserting that the TimeoutError? happens. Wilfredo seems to like the patter of adding errbacks to deferreds in test cases and calling self.fail from within them. I guess to give them an arguably more meaningful message.

[exarkun]

A hassle for the user might be a good thing, but this branch is probably the wrong place to make this policy decision. So by process of elimination we seem to have arrived at the third option. :P Other ideas?

"adaptable" is just what we want. Sorry, I didn't put that very clearly.

For test_waitUntilLockedWithTimeoutUnlocked - okay, that makes sense.

[dreid]

Ok, I fixed the docstring, added the defaultScheduler test, and a test for the error from a scheduler that isn't adaptable to IReactorTime.

[dreid]

Addressed the timeout issue.

[exarkun]

The time.time() call now present in deferUntilLocked is problematic. This shows up in the test case, too. You shouldn't need to set _started to 0 in test_waitUntilLockedWithTimeoutLocked (which will require the test to change when we switch to a non-polling implementation).

If there were a time method on IReactorTime, the obvious solution would be to call that instead. Since there isn't, I guess I'd suggest setting up one call for the full timeout interval when the initial deferUntilLocked call is made. When the lock is acquired, cancel that call and callback the result Deferred. If that call ever fires, cancel whatever poll call is pending and errback the result Deferred (maybe with one last lock() attempt, just to make sure it isn't going to succeed, and so that the time between the most recent poll attempt and the timeout isn't just wasted).

[exarkun]

• Clock doesn't actually implement IReactorTime :/ It needs cancelCallLater and getDelayedCalls. These should be straightforward to add, though.
• Please change if not scheduler: in DeferredFilesystemLock.__init__ to if scheduler is None:. The truth of the scheduler isn't relevant, just whether it was supplied. That it can be None to default to the reactor should probably be mentioned in the docstring as well.
• deferUntilLocked isn't safe for concurrent use anymore. This doesn't particularly bother me, but this should be documented and it should probably raise an exception if concurrent use is attempted.
• in _tryLock, please change if self._timeoutCall: to if self._timeoutCall is not None:.
• after a DelayedCall? is cancelled or fired, it's generally a good idea to drop the reference to that DelayedCall? (if one was saved). Beyond breaking reference cycles which make the gc work harder, it's a good idea in this case since the existence of these DelayedCalls? as attributes on the instance actually alters its behavior. Currently, a DeferredFilesystemLock? is only usable once: after the first run, self._timeoutCall is not None, so the timeout will not be set up, even if it is requested.

[dreid]

Replying to exarkun:

* Clock doesn't actually implement IReactorTime :/ It needs cancelCallLater and getDelayedCalls. These should be straightforward to add, though.

Done, i wasn't sure if I should bother with the deprecation warning on cancelCallLater, but I added it anyway.

* Please change if not scheduler: in DeferredFilesystemLock.__init__ to if scheduler is None:. The truth of the scheduler isn't relevant, just whether it was supplied. That it can be None to default to the reactor should probably be mentioned in the docstring as well.

Done

* deferUntilLocked isn't safe for concurrent use anymore. This doesn't particularly bother me, but this should be documented and it should probably raise an exception if concurrent use is attempted.

Done

* in _tryLock, please change if self._timeoutCall: to if self._timeoutCall is not None:.

Done

* after a DelayedCall? is cancelled or fired, it's generally a good idea to drop the reference to that DelayedCall? (if one was saved). Beyond breaking reference cycles which make the gc work harder, it's a good idea in this case since the existence of these DelayedCalls? as attributes on the instance actually alters its behavior. Currently, a DeferredFilesystemLock? is only usable once: after the first run, self._timeoutCall is not None, so the timeout will not be set up, even if it is requested.

Done, and added a testcase that shows it can be used multiple times.

[glyph]

While I do appreciate the fact that Clock now properly implements IReactorTime, I don't see why we need the additional complexity of adapting to IReactorTime. I also note that, ironically, there is no success-path test for adaptation, only a failure-path test. The test in question also has a questionable assert about its arguments, mandating the string format. I think it'd be better if the interface were just changed to say "... which provides ..." instead of "... adaptable to ...".

Don't use "assert" for the deferUntilLocked concurrency test. Make it an 'if', and don't make it an AssertionError, either. It's the sort of thing that application code might want to catch; AlreadyTryingToLock might be a better name. I think I would also prefer it if the error were a defer.fail, so that it could be documented explicitly as part of the Deferred interface. At the very least it should have an @raise if it's going to be a synchronous exception. It should also perhaps say a bit more about the scope of "concurrent use". You could, for example, create multiple DeferredFilesystemLock objects in the same process for the same name and use those concurrently just fine, it's just that one lock which is in a particular state. (Although, maybe just an exception name similar to the one I suggested and its docstring would explain that sufficiently.)

Just a nit, but, several docstrings seem to be wrapped at 59 characters. You could give them a bit more breathing room.

[dreid]

Ok, up for review in source:branches/lockfile-deferred-2180-2

[exarkun]

Could you delete the blank lines immediately following docstrings? It would also be nice to have a couple blank lines between each method.

DeferredFilesystemLock? class docstring is missing @ivars for its attributes.

deferUntilLocked documents timeout as an int. I hope floats are supported too. ;)

in the test module, test_defaultScheduler imports the reactor. There's no reason not to do this at module scope, though.

Many of the test docstrings use the word "exists" where they want to indicate that the lock is held. It would be nice to clean up this language a bit.

The test_task.py and task.py changes don't seem related. I suppose the addition of getDelayedCalls is alright (ideally this would be a separate ticket, though), but I don't think cancelCallLater should be added at all. No one should be using this API. On the off chance someone has some code that's 4 years old that hasn't been updated and they want to suddenly write tests for it using Clock, they can fix the code to use a non-deprecated API.

[dreid]

Replying to exarkun:

Could you delete the blank lines immediately following docstrings? It would also be nice to have a couple blank lines between each method.

Done.

DeferredFilesystemLock? class docstring is missing @ivars for its attributes.

DeferredFilesystemLock? doesn't have any public ivars other than those inherited from FilesystemLock?. Is there some mechanism for referencing the documentation of a superclass like this?

deferUntilLocked documents timeout as an int. I hope floats are supported too. ;)

Now documented as a float or an int and tested as such.

in the test module, test_defaultScheduler imports the reactor. There's no reason not to do this at module scope, though.

fixed

Many of the test docstrings use the word "exists" where they want to indicate that the lock is held. It would be nice to clean up this language a bit.

fixed.

The test_task.py and task.py changes don't seem related. I suppose the addition of getDelayedCalls is alright (ideally this would be a separate ticket, though), but I don't think cancelCallLater should be added at all. No one should be using this API. On the off chance someone has some code that's 4 years old that hasn't been updated and they want to suddenly write tests for it using Clock, they can fix the code to use a non-deprecated API.

moved to a seperate ticket #2783

[wsanchez]

Replying to dreid:

Replying to exarkun: DeferredFilesystemLock? doesn't have any public ivars other than those inherited from FilesystemLock?. Is there some mechanism for referencing the documentation of a superclass like this?

Agreed; it would be way better if pydoctor inherrited the docs from the superclass; copying them to subclasses is error-prone (if one update the super class, they aren't necessarily going to know all the subclasses to fix).

[exarkun]

DeferredFilesystemLock? has some private ivars. I don't really remember, but I think the documentation comment was in reference to those. It's useful to maintainers to have documentation for private things.

One test is failing:

[ERROR]: twisted.test.test_defer.DeferredFilesystemLockTestCase.test_defaultScheduler
Traceback (most recent call last):
  File "/scratch/scmikes/buildbots/twistedbuildbot/bot-scmikes-2.5/Twisted/twisted/test/test_defer.py", line 778, in test_defaultScheduler
    self.assertEquals(lock.scheduler, reactor)
exceptions.AttributeError: 'DeferredFilesystemLock' object has no attribute 'scheduler'

[dreid]

Fix failing test and document private ivars.

[exarkun]

The Private Instance Variables heading doesn't get turned into very nice HTML. I think the underscore prefix indicates privateness well enough; if you want though, put some words into each @ivar indicating that it is private? eg (private) or This is private.

The TimeoutError created in _cancelLock should have a message to help a developer identify its origin. Logging this failure looks like this:

        Traceback (most recent call last):
        Failure: twisted.internet.defer.TimeoutError:

which is quite a frustrating message. :)

The new time import in defer.py is no longer used, as is the IReactorTime import. Also an unused IReactorTime import in test_defer.py.

All pretty minor stuff. Feel free to merge after you fix these, if you want.

[dreid]

(In [21806]) Merge lockfile-deferred-2180-2: Add a DeferredFilesystemLock? which provides a

deferUntilLocked method.

The deferUntilLocked method allows a caller to wait for the aqcuisition of a filesystem lock can be aqcuired or a timeout is reached.

Author: wsanchez, dreid Reviewer: exarkun, glyph, therve Fixes #2180