Merge lp://staging/~jml/lazr.restfulclient/multiple-instance-safe into lp://staging/lazr.restfulclient

Thanks for this.

Jml:

This looks really great. I'm going to be that not-too-draconian reviewer and ask for the basic tests you've suggested.

I absolutely concur that creating a test for the race condition is out of consideration--the test case would like be sufficiently complicated that failures might be from a change to the test, not to the code being tested.

Next, I note several imports being whacked--I assume those are just not in use in the code anymore, correct?

Lastly, do you have landing rights for this, or will you need someone to push it along for you?

I'm very grateful for you addressing this bug.

This is very likely to fail on Windows due to the file being unable to be overwritten if it's in use. I know people have at least tried to use lazr.restfulclient and lplib on Windows. I don't know if it's actually supported or working. If it does work a bit, you might want to try not to regress it.

+1 for the LoC increase per
https://dev.launchpad.net/PolicyAndProcess/MaintenanceCosts - this is
a significant source of friction in using lazr.restful.

-Rob

Hello all,

Thanks for the reviews, and for the free pass on increasing LoC.

I've added interface tests for FileCache and have set these up to run against both httplib2.FileCache and AtomicFileCache. The difference it makes to test run time is pretty small relative to current run time, and it will prevent interface drift.

Adding these tests revealed one difference between FileCache and AtomicFileCache: if FileCache fails to set(), say, because of a TypeError, then it will return '' on get() for that key; AtomicFileCache will return None, as if the key were never set. I think the difference in behaviour doesn't actually matter for our purposes, but I've left the tests in as documentation anyway. I also think that AtomicFileCache behaves correctly here and that FileCache itself is wrong.

I have also added docstrings, which I hope will help.

Per Martin's comment, I've updated set() to remove the file before renaming it, following the pattern laid out in twisted.python.filepath (which has had extensive usage and testing on Windows).

JC, yes, the removed imports are unused. If you don't believe me, well, I guess the automated test suite will catch me out ;)

I don't have sufficient permissions to land this myself, nor do I actually know how to land it. (Is it PQM-managed? Tarmac? etc.) I would be grateful if you would land it for me.

cheers,
jml

> I've added interface tests for FileCache and have set these up to run against
> both httplib2.FileCache and AtomicFileCache. The difference it makes to test
> run time is pretty small relative to current run time, and it will prevent
> interface drift.
>
> Adding these tests revealed one difference between FileCache and
> AtomicFileCache: if FileCache fails to set(), say, because of a TypeError,
> then it will return '' on get() for that key; AtomicFileCache will return
> None, as if the key were never set. I think the difference in behaviour
> doesn't actually matter for our purposes, but I've left the tests in as
> documentation anyway. I also think that AtomicFileCache behaves correctly
> here and that FileCache itself is wrong.
>
> I have also added docstrings, which I hope will help.

Fantastic, thanks for those changes.

> JC, yes, the removed imports are unused. If you don't believe me, well, I
> guess the automated test suite will catch me out ;)

I believe you; I couldn't get at the code myself on Thursday for reasons best attributed to massive ISP fail, so fell back on the old method of asking. :-p

> I don't have sufficient permissions to land this myself, nor do I actually
> know how to land it. (Is it PQM-managed? Tarmac? etc.) I would be grateful if
> you would land it for me.

I will see what I can do; I think this is something we certainly want landed swiftly.
> cheers,
> jml

On 26 March 2012 15:16, j.c.sackett <email address hidden> wrote:
...
>> I don't have sufficient permissions to land this myself, nor do I actually
>> know how to land it. (Is it PQM-managed? Tarmac? etc.)  I would be grateful if
>> you would land it for me.
>
> I will see what I can do; I think this is something we certainly want landed swiftly.

Thanks! A release would help a lot too.

jml

How's it going getting this landed? (It's filling up a WIP slot on our kanban).

I don't want to stall this, but I do think that

    os.unlink(cache_full_path)

may still fail on Windows if the existing file is in use by some other client. Solutions could include:

 - just log it (perhaps silent by default) and continue
 - warn
 - retry for a bit (not great because it slows down the writer)
 - change to using win32 apis that don't lock against deletion

I don't know how well this works on Windows at the moment, but if people are using it this might be a regression.

On Mon, Apr 2, 2012 at 5:24 PM, Martin Pool <email address hidden> wrote:
> I don't want to stall this, but I do think that
>
>    os.unlink(cache_full_path)
>
> may still fail on Windows if the existing file is in use by some other client.  Solutions could include:
>
>  - just log it (perhaps silent by default) and continue
>  - warn
>  - retry for a bit (not great because it slows down the writer)
>  - change to using win32 apis that don't lock against deletion
>
> I don't know how well this works on Windows at the moment, but if people are using it this might be a regression.

It will die horribly at the moment, this isn't going to be a
regression: we don't currently support concurrent use. This patch will
give us concurrent use on Linux. A future iteration can do concurrent
use on win32.

-Rob