llvm dev - Nov 2019 - Updated polling logic in LockFileManager

Well, Yes.
There are a few differences with the https://reviews.llvm.org/D70563
 solution.
First of all, in D70563, in case if file will be deleted between `open` and
subscription to events, then there'll be maximum timeout. it seems that
kqueue is not validating file descriptor, so, in case if file descriptor is
already invalid, listening for a kqueue won't give any errors. It will just
wait for a timeout and won't receive any events.
This behavior can be simply simulated in the code sample, provided in
https://reviews.llvm.org/D70563

This is why in D70563 <https://reviews.llvm.org/D70563> registration to
the
queue and listening to are two different calls. So logic in the path is
1) Register for events  (now queue will  receive events, but we haven't
setup listeners to it yet)
2) Check if lock file was deleted
3) Start receiving events

This logic allows preventing race condition if the file was deleted in
between open/subscription calls.
It's very unlikely to have this race condition, but 90 seconds is pretty
long time.

Second. in  D70563 <https://reviews.llvm.org/D70563> there's an
additional
event we're listening to check if the process, lock owner has exited. So,
in case if owner has died, but for some reason hasn't deleted lock file,
we'll exit from the waiting queue.
D69575 solution will still wait for file to be deleted for a whole
90seoonds timeout.
I'm not sure, how often this lock owner dies without unlocking, but this
was in the original code, so I implemented this part as well.


On Fri, Nov 22, 2019 at 10:20 PM Michael Spencer <bigcheesegs at
gmail.com>
wrote:
> On Fri, Nov 22, 2019 at 10:59 AM TT KILEW via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> Hi there. I submitted a patch I llvm  that fixed polling logic in
>> LockFileManager
>> https://reviews.llvm.org/D70563
>>
>> This patch should fix
>> https://bugs.llvm.org/show_bug.cgi?id=20794
>>
>> There were no activity so I decided to write directly  to mail list
>>
>> Is there anyone who can take a look? Thanks.
>>
>> P.S. Are there some additional actions needed for patch to be reviewed?
>> Thanks
>>
>
> I recently reviewed https://reviews.llvm.org/D69575 which solves the same
> problem and seems simpler. Is there anything in your patch not covered by
> D69575?
>
> - Michael Spencer
>

-- 
Best Regards,
Павло Тайкало  / Paul Taykalo
skype: tt.kilew, tel.: +38 097 86 1024 1
http://twitter.com/tt_kilew
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191122/004e84ad/attachment.html>

I seem that my patch is duplicate of https://reviews.llvm.org/D69575, It's
just I covered some corner cases.
I think the best way to solve this is to cover some race conditions in
D69575 or simply ignore those corner cases if those are unlikely to happen.
I left some comments with explanations in https://reviews.llvm.org/D69575

On Fri, Nov 22, 2019 at 11:06 PM TT KILEW <tt.kilew at gmail.com> wrote:
> Well, Yes.
> There are a few differences with the https://reviews.llvm.org/D70563
>  solution.
> First of all, in D70563, in case if file will be deleted between `open`
> and subscription to events, then there'll be maximum timeout. it seems
that
> kqueue is not validating file descriptor, so, in case if file descriptor is
> already invalid, listening for a kqueue won't give any errors. It will
just
> wait for a timeout and won't receive any events.
> This behavior can be simply simulated in the code sample, provided in
> https://reviews.llvm.org/D70563
>
> This is why in D70563 <https://reviews.llvm.org/D70563> registration
to
> the queue and listening to are two different calls. So logic in the path is
> 1) Register for events  (now queue will  receive events, but we haven't
> setup listeners to it yet)
> 2) Check if lock file was deleted
> 3) Start receiving events
>
> This logic allows preventing race condition if the file was deleted in
> between open/subscription calls.
> It's very unlikely to have this race condition, but 90 seconds is
pretty
> long time.
>
> Second. in  D70563 <https://reviews.llvm.org/D70563> there's an
> additional event we're listening to check if the process, lock owner
has
> exited. So, in case if owner has died, but for some reason hasn't
deleted
> lock file, we'll exit from the waiting queue.
> D69575 solution will still wait for file to be deleted for a whole
> 90seoonds timeout.
> I'm not sure, how often this lock owner dies without unlocking, but
this
> was in the original code, so I implemented this part as well.
>
>
> On Fri, Nov 22, 2019 at 10:20 PM Michael Spencer <bigcheesegs at
gmail.com>
> wrote:
>
>> On Fri, Nov 22, 2019 at 10:59 AM TT KILEW via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>>> Hi there. I submitted a patch I llvm  that fixed polling logic in
>>> LockFileManager
>>> https://reviews.llvm.org/D70563
>>>
>>> This patch should fix
>>> https://bugs.llvm.org/show_bug.cgi?id=20794
>>>
>>> There were no activity so I decided to write directly  to mail list
>>>
>>> Is there anyone who can take a look? Thanks.
>>>
>>> P.S. Are there some additional actions needed for patch to be
reviewed?
>>> Thanks
>>>
>>
>> I recently reviewed https://reviews.llvm.org/D69575 which solves the
>> same problem and seems simpler. Is there anything in your patch not
covered
>> by D69575?
>>
>> - Michael Spencer
>>
>
>
> --
> Best Regards,
> Павло Тайкало  / Paul Taykalo
> skype: tt.kilew, tel.: +38 097 86 1024 1
> http://twitter.com/tt_kilew
>

-- 
Best Regards,
Павло Тайкало  / Paul Taykalo
skype: tt.kilew, tel.: +38 097 86 1024 1
http://twitter.com/tt_kilew
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191122/5867c7de/attachment.html>

There are seems no activity on this patch https://reviews.llvm.org/D69575
What is the best potential solution in this case?
Should I update own patch, or wait until the original will be merged in and
then apply fix?

Thanks in advance



On Fri, Nov 22, 2019 at 11:31 PM TT KILEW <tt.kilew at gmail.com> wrote:
> I seem that my patch is duplicate of https://reviews.llvm.org/D69575,
> It's just I covered some corner cases.
> I think the best way to solve this is to cover some race conditions in
> D69575 or simply ignore those corner cases if those are unlikely to happen.
> I left some comments with explanations in https://reviews.llvm.org/D69575
>
> On Fri, Nov 22, 2019 at 11:06 PM TT KILEW <tt.kilew at gmail.com>
wrote:
>
>> Well, Yes.
>> There are a few differences with the https://reviews.llvm.org/D70563
>>  solution.
>> First of all, in D70563, in case if file will be deleted between `open`
>> and subscription to events, then there'll be maximum timeout. it
seems that
>> kqueue is not validating file descriptor, so, in case if file
descriptor is
>> already invalid, listening for a kqueue won't give any errors. It
will just
>> wait for a timeout and won't receive any events.
>> This behavior can be simply simulated in the code sample, provided in
>> https://reviews.llvm.org/D70563
>>
>> This is why in D70563 <https://reviews.llvm.org/D70563>
registration to
>> the queue and listening to are two different calls. So logic in the
path is
>> 1) Register for events  (now queue will  receive events, but we
haven't
>> setup listeners to it yet)
>> 2) Check if lock file was deleted
>> 3) Start receiving events
>>
>> This logic allows preventing race condition if the file was deleted in
>> between open/subscription calls.
>> It's very unlikely to have this race condition, but 90 seconds is
pretty
>> long time.
>>
>> Second. in  D70563 <https://reviews.llvm.org/D70563> there's
an
>> additional event we're listening to check if the process, lock
owner has
>> exited. So, in case if owner has died, but for some reason hasn't
deleted
>> lock file, we'll exit from the waiting queue.
>> D69575 solution will still wait for file to be deleted for a whole
>> 90seoonds timeout.
>> I'm not sure, how often this lock owner dies without unlocking, but
this
>> was in the original code, so I implemented this part as well.
>>
>>
>> On Fri, Nov 22, 2019 at 10:20 PM Michael Spencer <bigcheesegs at
gmail.com>
>> wrote:
>>
>>> On Fri, Nov 22, 2019 at 10:59 AM TT KILEW via llvm-dev <
>>> llvm-dev at lists.llvm.org> wrote:
>>>
>>>> Hi there. I submitted a patch I llvm  that fixed polling logic
in
>>>> LockFileManager
>>>> https://reviews.llvm.org/D70563
>>>>
>>>> This patch should fix
>>>> https://bugs.llvm.org/show_bug.cgi?id=20794
>>>>
>>>> There were no activity so I decided to write directly  to mail
list
>>>>
>>>> Is there anyone who can take a look? Thanks.
>>>>
>>>> P.S. Are there some additional actions needed for patch to be
reviewed?
>>>> Thanks
>>>>
>>>
>>> I recently reviewed https://reviews.llvm.org/D69575 which solves
the
>>> same problem and seems simpler. Is there anything in your patch not
covered
>>> by D69575?
>>>
>>> - Michael Spencer
>>>
>>
>>
>> --
>> Best Regards,
>> Павло Тайкало  / Paul Taykalo
>> skype: tt.kilew, tel.: +38 097 86 1024 1
>> http://twitter.com/tt_kilew
>>
>
>
> --
> Best Regards,
> Павло Тайкало  / Paul Taykalo
> skype: tt.kilew, tel.: +38 097 86 1024 1
> http://twitter.com/tt_kilew
>

-- 
Best Regards,
Павло Тайкало  / Paul Taykalo
skype: tt.kilew, tel.: +38 097 86 1024 1
http://twitter.com/tt_kilew
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200210/f7199e5e/attachment.html>