On 08/23/2012 07:39 AM, Magnus Hagander wrote:
> On Thu, Aug 23, 2012 at 1:35 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>> On 08/23/2012 06:47 AM, Magnus Hagander wrote:
>>> On Mon, Aug 20, 2012 at 4:52 PM, Andrew Dunstan <andrew@dunslane.net>
>>> wrote:
>>>> Here is a patch for this feature, which should alleviate some of the woes
>>>> caused by adding labels not being transactional (and thus not allowing
>>>> for
>>>> the catching of errors).
>>> I haven't actually checked the code in detail, but if it's not
>>> transactional, how does it actually prevent race conditions? Doesn't
>>> it at least have to do it's check *after* the enum is locked?
>>
>>
>> Well, you can't remove a label, and if the test succeeds it results in your
>> doing nothing, so my possibly naive thinking was that that wasn't necessary.
>> But I could easily be wrong :-)
> Ah, good point.
> But still: what if:
>
> Session A checks if the label is present, it's not.
> Session B checks if the label is present, it's not.
> Session A locks the enum, and adds the label, then releases lock.
> Session B locks the enum, and tries to add it -- and you still get a failure.
>
> It doesn't break, of course ,since it's protected by the unique index.
> But aren't you at risk of getting the very error message you're trying
> to avoid?
>
> Or am I missing something? (I probably am :D - I still haven't looked
> at it in detail)
>
Yeah, looking further this was probably a thinko on my part. Thanks for
noticing. I've moved the test down so it's done right after the lock is
acquired. Revised patch attached.
cheers
andrew