Code review comment for lp://staging/~chipaca/snappy/lock-ness

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

Gustavo Niemeyer, [21.10.15 10:16]
@chipaca I'm lacking some context for why we'd want that locked-locking-locker

John Lenton, [21.10.15 10:17]
@niemeyer you mean the mutex inside the mmutex, or you mean the whole branch?

Gustavo Niemeyer, [21.10.15 10:17]
I mean the whole thing

John Lenton, [21.10.15 10:18]
@niemeyer well, we need some kind of a lock, to avoid people doing things at the same time and putting us in an unknown state

John Lenton, [21.10.15 10:19]
@niemeyer and a single, global lock didn't seem like a good idea

Gustavo Niemeyer, [21.10.15 10:19]
@chipaca In general we try to accommodate for people doing things at the same time instead, and prevent the actual critical things that cannot be mutated at once from being mutated at once

John Lenton, [21.10.15 10:19]
@niemeyer yes, in general we do

Gustavo Niemeyer, [21.10.15 10:20]
@chipaca Right, thus why I'm trying to understand how this locking system fits

John Lenton, [21.10.15 10:24]
@niemeyer it fits in that most of snappy is still the commandline one-shot thing, which has a filesystem-based lock by necessity to prevent different invocations of it to collide

John Lenton, [21.10.15 10:25]
@niemeyer a refactor of the level needed to move it to accomplish this exclusion without explicit locks is beyond the scope of the work we can do right now

Gustavo Niemeyer, [21.10.15 10:25]
@Chipaca So keep the filesystem locks?

Gustavo Niemeyer, [21.10.15 10:26]
@chipaca Being in a hurry and doing a big complex locking system doesn't feel compatible

John Lenton, [21.10.15 10:27]
this isn't a big complex locking system, imo

John Lenton, [21.10.15 10:27]
the filesystem lock is still there

Gustavo Niemeyer, [21.10.15 10:27]
@Chipaca Rather than one big central system, it would feel much better to me to give the proper places in the code authority for their own concurrency handling

John Lenton, [21.10.15 10:28]
@niemeyer isn't that what this does?

Gustavo Niemeyer, [21.10.15 10:29]
@chipaca That often amounts to a single local mutex, if at all

Gustavo Niemeyer, [21.10.15 10:29]
@chipaca So, again, I may well not be aware of the use case, but it feels like the use case is what we should be talking about

Gustavo Niemeyer, [21.10.15 10:30]
@chipaca It feels complex to me, and it feels complex to mvo, FWIW

John Lenton, [21.10.15 10:31]
yes, but less than 200 lines of code including whitespace and comments is not "big and complex"

John Lenton, [21.10.15 10:31]
anyway, the use case

Gustavo Niemeyer, [21.10.15 10:32]
@chipaca mutex.go is 126 lines long

Gustavo Niemeyer, [21.10.15 10:32]
and a lot of it is docs :)

Gustavo Niemeyer, [21.10.15 10:33]
But I digress

John Lenton, [21.10.15 10:33]
plus 137 for rwmutex

Gustavo Niemeyer, [21.10.15 10:33]
The key here is the use case.. if we need it, so be it, but I'm not yet understanding how this would fit in that cannot be solved by something simpler

John Lenton, [21.10.15 10:34]
when somebody we need a list of packages, or services, etc, we need to avoid people installing/removing/starting/stopping things for a bit

John Lenton, [21.10.15 10:34]
when people operate on a package, we need to stop people operating on that package for a bit

John Lenton, [21.10.15 10:34]
that's about it

John Lenton, [21.10.15 10:35]
the first is the 'root' lock, the second is the leaf lock

John Lenton, [21.10.15 10:35]
in my code

Gustavo Niemeyer, [21.10.15 10:39]
@chipaca I'm really lacking context for telling whether that's a good idea or not, but my initial gut feeling is that this is a lot of logic for what I've seen being solved with a simple mutex in a local value that has the authority for the given area

Gustavo Niemeyer, [21.10.15 10:39]
@chipaca The best way to solve this is likely for the two of us to sit together and look at some use cases in detail

review: Needs Information

« Back to merge proposal