diff mbox series

[4/6] event: builtins fix for 'd' deletion

Message ID 20221220151604.415637-4-richard.purdie@linuxfoundation.org
State Accepted, archived
Commit 1e12f0a4b592dacd006d370ec29cd71d2a44312e
Headers show
Series [1/6] knotty: Ping the server/cooker periodically | expand

Commit Message

Richard Purdie Dec. 20, 2022, 3:16 p.m. UTC
I've been seeing event handlers where 'd' seems to disappear half way through
event handler execution. This is problematic when multiple threads are active
since this code assumes single threading. Add in a recursive lock to avoid
d being deleted from a context it shouldn't be.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 lib/bb/event.py | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Joshua Watt Dec. 20, 2022, 4:41 p.m. UTC | #1
On Tue, Dec 20, 2022 at 9:16 AM Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
>
> I've been seeing event handlers where 'd' seems to disappear half way through
> event handler execution. This is problematic when multiple threads are active
> since this code assumes single threading. Add in a recursive lock to avoid
> d being deleted from a context it shouldn't be.
>
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>  lib/bb/event.py | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/lib/bb/event.py b/lib/bb/event.py
> index 21e9a1b025..5df5b3a3ee 100644
> --- a/lib/bb/event.py
> +++ b/lib/bb/event.py
> @@ -68,6 +68,7 @@ _catchall_handlers = {}
>  _eventfilter = None
>  _uiready = False
>  _thread_lock = threading.Lock()
> +_builtins_lock = threading.RLock()

Does this need to be a recursive lock?

>  _thread_lock_enabled = False
>  _heartbeat_enabled = False
>
> @@ -93,6 +94,9 @@ def disable_heartbeat():
>      _heartbeat_enabled = False
>
>  def execute_handler(name, handler, event, d):
> +    if _thread_lock_enabled:
> +        _builtins_lock.acquire()
> +
>      event.data = d
>      addedd = False
>      if 'd' not in builtins:
> @@ -115,6 +119,9 @@ def execute_handler(name, handler, event, d):
>          del event.data
>          if addedd:
>              del builtins['d']
> +        if _thread_lock_enabled:
> +            _builtins_lock.release()
> +
>
>  def fire_class_handlers(event, d):
>      if isinstance(event, logging.LogRecord):
> --
> 2.37.2
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#14212): https://lists.openembedded.org/g/bitbake-devel/message/14212
> Mute This Topic: https://lists.openembedded.org/mt/95787500/3616693
> Group Owner: bitbake-devel+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/bitbake-devel/unsub [JPEWhacker@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
Richard Purdie Dec. 20, 2022, 4:47 p.m. UTC | #2
On Tue, 2022-12-20 at 10:41 -0600, Joshua Watt wrote:
> On Tue, Dec 20, 2022 at 9:16 AM Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > 
> > I've been seeing event handlers where 'd' seems to disappear half way through
> > event handler execution. This is problematic when multiple threads are active
> > since this code assumes single threading. Add in a recursive lock to avoid
> > d being deleted from a context it shouldn't be.
> > 
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > ---
> >  lib/bb/event.py | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/lib/bb/event.py b/lib/bb/event.py
> > index 21e9a1b025..5df5b3a3ee 100644
> > --- a/lib/bb/event.py
> > +++ b/lib/bb/event.py
> > @@ -68,6 +68,7 @@ _catchall_handlers = {}
> >  _eventfilter = None
> >  _uiready = False
> >  _thread_lock = threading.Lock()
> > +_builtins_lock = threading.RLock()
> 
> Does this need to be a recursive lock?

Yes, in testing these can be re-enterant (think an event handler that
does a bb.warn()) so yes, it does need to be.

Cheers,

Richard
diff mbox series

Patch

diff --git a/lib/bb/event.py b/lib/bb/event.py
index 21e9a1b025..5df5b3a3ee 100644
--- a/lib/bb/event.py
+++ b/lib/bb/event.py
@@ -68,6 +68,7 @@  _catchall_handlers = {}
 _eventfilter = None
 _uiready = False
 _thread_lock = threading.Lock()
+_builtins_lock = threading.RLock()
 _thread_lock_enabled = False
 _heartbeat_enabled = False
 
@@ -93,6 +94,9 @@  def disable_heartbeat():
     _heartbeat_enabled = False
 
 def execute_handler(name, handler, event, d):
+    if _thread_lock_enabled:
+        _builtins_lock.acquire()
+
     event.data = d
     addedd = False
     if 'd' not in builtins:
@@ -115,6 +119,9 @@  def execute_handler(name, handler, event, d):
         del event.data
         if addedd:
             del builtins['d']
+        if _thread_lock_enabled:
+            _builtins_lock.release()
+
 
 def fire_class_handlers(event, d):
     if isinstance(event, logging.LogRecord):