[bitbake-devel] server/process: Fix a rare lockfile race

Submitted by Richard Purdie on July 12, 2020, 10:55 a.m. | Patch ID: 174405

Details

Message ID 20200712105516.507243-1-richard.purdie@linuxfoundation.org
State Master Next
Commit bbc20bafb80b19182f7a765429cc8d5d68d10223
Headers show

Commit Message

Richard Purdie July 12, 2020, 10:55 a.m.
We're seeing rare occasional races on the autobuilder as if two server
processes have the lockfile at the same time. We need to be extremely
careful this does not happen.

I think there is a potential race in this shutdown code since we delete
the lockfile, then call unlockfile() which also tries to delete it.

This means we may remove a lock file now held by another process if we're
unlucky. Since unlockfile removes the lockfile when it can, just rely on
that and remove any possible race window.

An example cooker-deamonlog:

--- Starting bitbake server pid 2266 at 2020-07-11 06:17:18.210777 ---
Started bitbake server pid 2266
Entering server connection loop
Accepting [<socket.socket fd=20, family=AddressFamily.AF_UNIX, type=SocketKind.SOCK_STREAM, proto=0, laddr=bitbake.sock>] ([])
Processing Client
Connecting Client
Running command ['setFeatures', [2]]
Running command ['updateConfig', XXX]
Running command ['getVariable', 'BBINCLUDELOGS']
Running command ['getVariable', 'BBINCLUDELOGS_LINES']
Running command ['getSetVariable', 'BB_CONSOLELOG']
Running command ['getSetVariable', 'BB_LOGCONFIG']
Running command ['getUIHandlerNum']
Running command ['setEventMask', XXXX]
Running command ['getVariable', 'BB_DEFAULT_TASK']
Running command ['setConfig', 'cmd', 'build']
Running command ['getVariable', 'BBTARGETS']
Running command ['parseFiles']
--- Starting bitbake server pid 8252 at 2020-07-11 06:17:28.584514 ---
Started bitbake server pid 8252
--- Starting bitbake server pid 13278 at 2020-07-11 06:17:31.330635 ---
Started bitbake server pid 13278
Running command ['dataStoreConnectorCmd', 0, 'getVar', ('BBMULTICONFIG',), {}]
Running command ['getRecipes', '']
Running command ['clientComplete']
Processing Client
Disconnecting Client
No timeout, exiting.
Exiting

where it looks like there are two server processes running which should not be.
In that build there was a process left sitting in memory with its bitbake.sock file
missing but holding the lock (not sure why it wouldn't timeout/exit).

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 lib/bb/server/process.py | 1 -
 1 file changed, 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/lib/bb/server/process.py b/lib/bb/server/process.py
index 83385baf60..b3a7f8b419 100644
--- a/lib/bb/server/process.py
+++ b/lib/bb/server/process.py
@@ -243,7 +243,6 @@  class ProcessServer(multiprocessing.Process):
                 lock = bb.utils.lockfile(lockfile, shared=False, retry=False, block=True)
                 if lock:
                     # We hold the lock so we can remove the file (hide stale pid data)
-                    bb.utils.remove(lockfile)
                     bb.utils.unlockfile(lock)
                     return
 

Comments

Jacob Kroon July 12, 2020, 12:05 p.m.
On 7/12/20 12:55 PM, Richard Purdie wrote:
> We're seeing rare occasional races on the autobuilder as if two server
> processes have the lockfile at the same time. We need to be extremely
> careful this does not happen.
> 
> I think there is a potential race in this shutdown code since we delete
> the lockfile, then call unlockfile() which also tries to delete it.
> 
> This means we may remove a lock file now held by another process if we're
> unlucky. Since unlockfile removes the lockfile when it can, just rely on
> that and remove any possible race window.
> 
> An example cooker-deamonlog:
> 
> --- Starting bitbake server pid 2266 at 2020-07-11 06:17:18.210777 ---
> Started bitbake server pid 2266
> Entering server connection loop
> Accepting [<socket.socket fd=20, family=AddressFamily.AF_UNIX, type=SocketKind.SOCK_STREAM, proto=0, laddr=bitbake.sock>] ([])
> Processing Client
> Connecting Client
> Running command ['setFeatures', [2]]
> Running command ['updateConfig', XXX]
> Running command ['getVariable', 'BBINCLUDELOGS']
> Running command ['getVariable', 'BBINCLUDELOGS_LINES']
> Running command ['getSetVariable', 'BB_CONSOLELOG']
> Running command ['getSetVariable', 'BB_LOGCONFIG']
> Running command ['getUIHandlerNum']
> Running command ['setEventMask', XXXX]
> Running command ['getVariable', 'BB_DEFAULT_TASK']
> Running command ['setConfig', 'cmd', 'build']
> Running command ['getVariable', 'BBTARGETS']
> Running command ['parseFiles']
> --- Starting bitbake server pid 8252 at 2020-07-11 06:17:28.584514 ---
> Started bitbake server pid 8252
> --- Starting bitbake server pid 13278 at 2020-07-11 06:17:31.330635 ---
> Started bitbake server pid 13278
> Running command ['dataStoreConnectorCmd', 0, 'getVar', ('BBMULTICONFIG',), {}]
> Running command ['getRecipes', '']
> Running command ['clientComplete']
> Processing Client
> Disconnecting Client
> No timeout, exiting.
> Exiting
> 
> where it looks like there are two server processes running which should not be.
> In that build there was a process left sitting in memory with its bitbake.sock file
> missing but holding the lock (not sure why it wouldn't timeout/exit).
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>   lib/bb/server/process.py | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/lib/bb/server/process.py b/lib/bb/server/process.py
> index 83385baf60..b3a7f8b419 100644
> --- a/lib/bb/server/process.py
> +++ b/lib/bb/server/process.py
> @@ -243,7 +243,6 @@ class ProcessServer(multiprocessing.Process):
>                   lock = bb.utils.lockfile(lockfile, shared=False, retry=False, block=True)
>                   if lock:
>                       # We hold the lock so we can remove the file (hide stale pid data)
> -                    bb.utils.remove(lockfile)
>                       bb.utils.unlockfile(lock)
>                       return
>   

I'm no export on the bb lockfiles, but if this is correct we should 
update the comment aswell.

Jacob
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.

View/Reply Online (#11488): https://lists.openembedded.org/g/bitbake-devel/message/11488
Mute This Topic: https://lists.openembedded.org/mt/75454627/3617530
Group Owner: bitbake-devel+owner@lists.openembedded.org
Unsubscribe: https://lists.openembedded.org/g/bitbake-devel/unsub  [oe-patchwork@oe-patch.openembedded.org]
-=-=-=-=-=-=-=-=-=-=-=-
Richard Purdie July 12, 2020, 1:14 p.m.
On Sun, 2020-07-12 at 14:05 +0200, Jacob Kroon wrote:
> On 7/12/20 12:55 PM, Richard Purdie wrote:
> > diff --git a/lib/bb/server/process.py b/lib/bb/server/process.py
> > index 83385baf60..b3a7f8b419 100644
> > --- a/lib/bb/server/process.py
> > +++ b/lib/bb/server/process.py
> > @@ -243,7 +243,6 @@ class ProcessServer(multiprocessing.Process):
> >                   lock = bb.utils.lockfile(lockfile, shared=False,
> > retry=False, block=True)
> >                   if lock:
> >                       # We hold the lock so we can remove the file
> > (hide stale pid data)
> > -                    bb.utils.remove(lockfile)
> >                       bb.utils.unlockfile(lock)
> >                       return
> >   
> 
> I'm no export on the bb lockfiles, but if this is correct we should 
> update the comment aswell.

I left the comment as its still right in that unlockfile does remove
the stale pid data potentially but I guess it could be tweaked to
better match the code...

Cheers,

Richard
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.

View/Reply Online (#11489): https://lists.openembedded.org/g/bitbake-devel/message/11489
Mute This Topic: https://lists.openembedded.org/mt/75454627/3617530
Group Owner: bitbake-devel+owner@lists.openembedded.org
Unsubscribe: https://lists.openembedded.org/g/bitbake-devel/unsub  [oe-patchwork@oe-patch.openembedded.org]
-=-=-=-=-=-=-=-=-=-=-=-