Merge lp://staging/~vila/bzr/525571-lock-bazaar-conf-files into lp://staging/bzr

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...

> Note that since we use an atomic write we neglect to protect
> readers against concurrent writers. This shouldn't be a problem
> in practice and I've been yelled at for suggesting it in a
> paranoid attempt to cover all cases. The case at point being:
>
> - a reader opens a file,
> - starts to read it (unlikely to not complete in a single IO),
> - a writer somehow manages to replace the opened file (unlikely since we
> do a rename),
> - the OS presents the new content to the reader.
>
> Since I'm not even sure this scenario is valid, I'll wait for
> evidence before considering it.

POSIX systems require that the currently open file stays pointing at the
same content (same inode). While the rename replaces the directory
information, the open file handle does not point to a new file. (You
would have to have 'read' be implemented as (open(), seek(), read(),
close(), to get this behavior.)

Windows says that open() creates a lock on the *path*, which means that
you cannot rename a new file over a file which is already open. (So no
atomic rename operation.)

There is probably a CreateFile flag that might allow it (as there is one
that allows you to delete an open file IIRC). But we don't use it ourselves.

I'm about 95% sure that this scenario is invalid.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkwrrRMACgkQJdeBCYSNAANG5QCeNjIy3IeDz2fFMW0KPHEUp5qv
gdYAoMG9ymYJX32g3tVRWCsIXF5vke8r
=nu81
-----END PGP SIGNATURE-----

16:24 < lifeless> vila: --config please, not --conf
16:25 < lifeless> --conf is a strange abbreviation
16:25 < lifeless> c = config.LockableConfig(lambda : location)
16:25 < lifeless> reads really strangely
16:25 < lifeless> perhaps the LockableConfig constructor should be different
16:27 < vila> lifeless: I don't want to break the chain of constructors, I can add a def get_filename if you prefer

When a class provokes ugly code in users, the class is wrong. Perhaps the class could take two keyword constructors, and then only the new code in builtins uses a keyword, and uses the second one to supply a name.

the class can then use it.

The ironic thing here is the first thing the class does is make the filename by calling the constructor.

Please note in the (otherwise excellent) docstring that the reason callers need to lock_write, rather than the innermost writer function, is to ensure that the file is both *read* and *written* in the same lock.

Perhaps that would be best tracked - when doing a read, if the object is not locked, set self._can_write = False, and if it is, set it to True. Then check that as well as whether it is locked, in _write_config.

The file=None to infile=None change seems to just generate spurious noise, and is not done in a backward compatible way.

We are likely going to want to backport this for launchpad, so it really should be a focused patch.

Likewise the test changes that are not related to locking consistency.

Please roll those things back, and I think we'll have a much more focused patch that we can confidently get LP to cherrypick.

There's also a number of opaque variable names like 'c', 'p' and 'lc_tests' - while I figured out what they were, they could be a lot clearer.

> 16:24 < lifeless> vila: --config please, not --conf
> 16:25 < lifeless> --conf is a strange abbreviation
> 16:25 < lifeless> c = config.LockableConfig(lambda : location)
> 16:25 < lifeless> reads really strangely
> 16:25 < lifeless> perhaps the LockableConfig constructor should be different
> 16:27 < vila> lifeless: I don't want to break the chain of constructors, I can
> add a def get_filename if you prefer
>
>
> When a class provokes ugly code in users, the class is wrong. Perhaps the
> class could take two keyword constructors, and then only the new code in
> builtins uses a keyword, and uses the second one to supply a name.

I.e. __init__ should accept file_name and get_filename and deprecate the later ?

>
> the class can then use it.

Conflicts with making a focused patch. I'll keep that for an updated submission focused on cleanup.

>
> The ironic thing here is the first thing the class does is make the filename
> by calling the constructor.

Yup, it's needed to build the lock directory (I'm sure you got that). The other classes postpone the get_filename() evaluation until it's needed, but given the functions used to provide the names, I don't think postponing is really required (some env variables are involved though, so there may be edge cases).

>
> Please note in the (otherwise excellent) docstring that the reason callers
> need to lock_write, rather than the innermost writer function, is to ensure
> that the file is both *read* and *written* in the same lock.
>

I'll mention that. And yes, it's the real reason for requiring a larger lock scope.

> Perhaps that would be best tracked - when doing a read, if the object is not
> locked, set self._can_write = False, and if it is, set it to True. Then check
> that as well as whether it is locked, in _write_config.

_can_write == is_locked() I see the idea but it won't change the end result nor the feedback we can give.

>
>
> The file=None to infile=None change seems to just generate spurious noise, and
> is not done in a backward compatible way.

It's on a private method and used only by tests (and provide a poor way to build test configs anyway).

>
> We are likely going to want to backport this for launchpad, so it really
> should be a focused patch.

I'll restart from scratch then.

>
> Likewise the test changes that are not related to locking consistency.

Most of them were required to address the flawed assumption that a config object (with content)
can be created without an actual file existing, I had to fix the failing tests.
The duplicated ensure_config_dir_exists() came in the way to.

And I mention that to explain that many cleanups were not for the sake of pure cleaning but driven by problems making the fix harder.

I'll look into an alternative solution to minimise the patch but that would likely end up in either weird code or intrusive changes, we'll see.

> 16:24 < lifeless> vila: --config please, not --conf
> 16:25 < lifeless> --conf is a strange abbreviation

:-P

Vila, I think I need to expand on this:

>> Perhaps that would be best tracked - when doing a read, if the object is not
>> locked, set self._can_write = False, and if it is, set it to True. Then check
>> that as well as whether it is locked, in _write_config.

> _can_write == is_locked() I see the idea but it won't change the end result nor the feedback we can give.

They are quite different.

If when you *read* you are unlocked, and you permit a *write* without forcing a new read, changes from other processes are discarded.

Here:
conf.read_something()
--client 2 writes to the file
conf.set_something()
--*boom* client 2's changes are lost

What we need is two fold: detection of this case (which setting 'can write' during *read* if it is write locked at the time will permit), and easy api use (which may be less immediate depending on impact).

Alternatively lock_write could possibly trigger a read after locking before returning, which would cause properly lock_write() guarded mutators to work correctly - as they would apply the mutation within the locked context.

In short - concurrency is hard, lets drink beer.

On Sun, Jul 4, 2010 at 6:39 PM, Robert Collins
<email address hidden> wrote:
[snip]
> In short - concurrency is hard, lets drink beer.

I think that's the best quote I've seen in a while! :-)

-John

>>>>> Robert Collins <email address hidden> writes:

    > Review: Needs Fixing
    > Vila, I think I need to expand on this:

    >>> Perhaps that would be best tracked - when doing a read, if the object is not
    >>> locked, set self._can_write = False, and if it is, set it to True. Then check
    >>> that as well as whether it is locked, in _write_config.

    >> _can_write == is_locked() I see the idea but it won't change the end result nor the feedback we can give.

    > They are quite different.

    > If when you *read* you are unlocked, and you permit a *write*
    > without forcing a new read,

Which this proposal guards against.

    > changes from other processes are discarded.

Yes, that's a delicate problem. This proposal isn't perfect but good
enough to cover most of our use cases.

    > Here:
    > conf.read_something()
    > --client 2 writes to the file
    > conf.set_something()
    > --*boom* client 2's changes are lost

Yup, I added a test for that and saw it fail.

    > What we need is two fold: detection of this case (which setting
    > 'can write' during *read* if it is write locked at the time will
    > permit),

Right, but what will we gain here ? The ability to *not* force the
refreshing read if someone already did it while write locked ?

Yet, it goes in the same direction as implementing a no-op read lock,
which we may want to add too, so, worth considering.

    > and easy api use (which may be less immediate depending on
    > impact).

    > Alternatively lock_write could possibly trigger a read after
    > locking before returning, which would cause properly lock_write()
    > guarded mutators to work correctly - as they would apply the
    > mutation within the locked context.

Inside the lock scope, a write should always happen *after* a refreshing
read or you'll lose changes for variables you're not changing (the
current API change one variable at a time).

Of course you always lose any change to the variable you're changing and
that in itself could be a problem if processes try to use config
variables for synchronization (a really bad idea), like an incremented
counter.

    > In short - concurrency is hard, lets drink beer.

+1

There are various lock schemes with different scopes that can be tried,
but so far, I haven't found a good way to address:

- client1 read 'var'
- client1 starts a "long" process based on 'var'
- client2 set a new value for 'var'
- client1 finished its process with a now outdated value for 'var'

Short of blocking client2 for the duration of client1 process, I can't
see how to address this.

But do we care ?

It's hard to decide without concrete examples, if 'var' is a parent
branch location and the process is pull, chances are the user really
want its pull to finish and changing 'var' to point to a new branch
should just be considered the next time we try to pull (alternate
scenarios includes: abort the pull (and rollback the already fetched
revisions (eeerk)), block the write (i.e. block the whole config file
for the duration of client1 process (omg are you crazy ?))).

Overall, I agree that we should continue thinking about which lock model
we want here, the proposed...

Marking as WIP pending a discussion during our next sprint on larger issues.
A minimal fix has landed for 2.1 using atomic writes which should be a good enough fix in the mean time.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...

>
> Overall, I agree that we should continue thinking about which lock model
> we want here, the proposed one is incrementally better and at least
> address the bug better than the minimal one landed for 2.1.
>
> Orthogonal to the lock scope, sharing a config files at the
> wt/branch/repo level could at least avoid reading the config file for
> each variable which could make read locks less painful, but still, I
> don't think it would be enough.

I think defining a scope is reasonable, and just sticking with that.

IMO, configs could be treated just like the rest of the branch data. So
if you '.lock_read()' at the beginning of a process, you read the config
state at that point in time, and then never read the config file again.

When you go to write, you probably need to re-read the file, because a
given text file has more content than what you are likely to be adjusting.

We *could* implement some sort of CAS to make config updates atomic. I
don't think that is worthwhile. (I tried to overwrite parent X with
parent Y, but parent was already Z.)

I agree that you shouldn't try to abuse configs to store incremental
counters, etc.

I honestly don't know what state was getting trashed by concurrent
updates, but I have the feeling that all of the bzr core data doesn't
really care. (Yes, you might get the wrong parent pointer, or submit
pointer, but we don't really store 'live' data in conf files.)

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkwzP7gACgkQJdeBCYSNAANa5QCdFVLPjMHJLyKn7MvR9NdkiAhp
6r8An3u2enhCMDQVbPS/wOmeo5LUO6X3
=Kmd7
-----END PGP SIGNATURE-----

Superseded by the loom ending in https://code.edge.launchpad.net/~vila/bzr/remove-gratuitous-ensure-config-dir-exist-calls/+merge/33426