Timed lock aquisition #5599

mcdeoliveira · 2020-02-01T06:46:15Z

This PR handles the timeout parameter in thread_lock_acquire.

It provides a set of compilation flags, tests, and an implementation for the unix and the esp32 ports. Other ports based on freertos should be easy to implement based on the esp32 port, because freertos lock_acquire already handles the timeout parameter. Unfortunately I do not have access to other hardware, so I think it is better to leave to others the task of replicating the functionality on other ports.

Let me just add a word about the relevance of handling the timeout parameter in lock_acquire. The only feature used by the current CPython implementation of threading.py that is nor yet supported by _thread is precisely a call to lock_acquire with a timeout parameter. In fact, a _thread_lock_acquire with timeout is issued in RLock and all subsequent classes in threading.py are based on RLock! Implementing the timeout parameter essentially enables a micropython version of Threading.

# Conflicts: # ports/unix/mpconfigport.h

dpgeorge · 2020-02-01T11:38:57Z

Thanks for the contribution. In general I'd be supportive of adding this feature. But please see previous discussion about this at #3332, and also try to fix the Travis CI failures.

dlech

Glad to see this getting implemented. 😄

I also like the suggestion of just modifying mp_thread_mutex_lock instead of adding a new mp_thread_mutex_lock_timed function. This would simplify things a bit.

dlech · 2020-02-01T16:47:17Z

ports/esp32/mpconfigport.h

@@ -123,6 +123,7 @@
 #define MICROPY_PY_THREAD                   (1)
 #define MICROPY_PY_THREAD_GIL               (1)
 #define MICROPY_PY_THREAD_GIL_VM_DIVISOR    (32)
+#define MICROPY_PY_THREAD_TIMEDLOCK         (1)


It might be nice to name this after the python feature rather than the posix thread feature, e.g. MICROPY_PY_THREAD_LOCK_TIMEOUT

dlech · 2020-02-01T16:49:26Z

ports/unix/Makefile

@@ -123,6 +123,9 @@ ifeq ($(MICROPY_PY_THREAD),1)
 CFLAGS_MOD += -DMICROPY_PY_THREAD=1 -DMICROPY_PY_THREAD_GIL=0
 LDFLAGS_MOD += $(LIBPTHREAD)
 endif
+ifeq ($(MICROPY_PY_THREAD_TIMEDLOCK),1)


This new configuration option doesn't really need to be in the Makefile. It could go in mpconfigport.h instead.

The only reason MICROPY_PY_THREAD is in the Makefile is because it affects the compiler flags.

py/modthread.c

+#ifdef MICROPY_PY_THREAD_TIMEDLOCK
+        ret = mp_thread_mutex_lock_timed(&self->mutex, timeout);
+#else
+        mp_raise_ValueError("timeout not supported");


dlech · 2020-02-01T17:07:47Z

ports/unix/mpthreadport.c

+    struct timeval _timeval;
+    struct timezone _timezone;
+    gettimeofday(&_timeval, &_timezone);
+    uint32_t _timeout_nano = ((uint32_t) (1e9*timeout)) + 1000 * _timeval.tv_usec;


Maybe do the float conversion like ports/unix/modtime.c?

struct timeval tv; mp_float_t val = mp_obj_get_float(arg); double ipart; tv.tv_usec = round(modf(val, &ipart) * 1000000); tv.tv_sec = ipart;

Otherwise, _timeout_nano probably needs to be uint64_t

In the updated version I am handling the float to int conversion in modthread.c and passing an int number of milliseconds to mp_thread_mutex_lock_timeout. See my comment regarding whether one should support int and float as timeout argument.

Regarding the overflow issue, the updated version is safe with uint32_t because _timeout_nano will always be smaller than 2 seconds.

dlech · 2020-02-01T17:13:47Z

tests/thread/thread_locktimed2.py

@@ -0,0 +1,34 @@
+# test timed _thread lock object using a single thread


Should this say "multiple threads" instead of "single thread"?

mcdeoliveira · 2020-02-01T19:09:00Z

Glad to see this getting implemented.

I also like the suggestion of just modifying mp_thread_mutex_lock instead of adding a new mp_thread_mutex_lock_timed function. This would simplify things a bit.

The one problem I see with this is that mp_thread_mutex_lock is used a lot throughout the code and I am afraid a change here could leave some undetected side effects. The semantics of the call would be different and a wait of 1 would have to be changed to a negative timeout_ms. How about something like this:

#ifdef MICROPY_PY_THREAD_LOCK_TIMEOUT
int mp_thread_mutex_lock_timed(mp_thread_mutex_t *mutex, int timeout_ms);
#define mp_thread_mutex_lock(mutex, wait) mp_thread_mutex_lock_timed(mutex, wait ? -1 : 0 )
#else
int mp_thread_mutex_lock(mp_thread_mutex_t *mutex, int wait);
#endif

This way the existing calls to mp_thread_mutex_lock would continue to be valid and ports supporting the timed option would only have to implement one function.

mcdeoliveira · 2020-02-01T19:10:38Z

Glad to see this getting implemented.

I also like the suggestion of just modifying mp_thread_mutex_lock instead of adding a new mp_thread_mutex_lock_timed function. This would simplify things a bit.

Thanks for all the great suggestions!

- label as mp_thread_mutex_lock_timeout - change signature to avoid handling floats on the ports - fixing possible overflow in unix port implementation - removing changes in Makefile for unix port - implementation for cc3200 - no longer raising exception when GIL lock is released

mcdeoliveira · 2020-02-02T01:04:36Z

Thanks for the contribution. In general I'd be supportive of adding this feature. But please see previous discussion about this at #3332, and also try to fix the Travis CI failures.

The only failure at Travis CI after the update is the growth in core size in the "bare-arm and minimal ports" build.

@dpgeorge I also have a question regarding thread safety in micropython. The first four concurrency tests, the mutate_* files, in the thread testing directory all fail. I was under the impression that the GIL would automatically prevent concurrent data access but this does not seem to be the case. Should that be fixed with a local mutex. Any thoughts?

Finally, the other test that fails is thread_exc2.py. This one seems to be hard to perform because the output produced by the uncaught exception will vary depending on the python version you're using.

mcdeoliveira

Going from ms to us. ms timeout can be too inaccurate if used in clocks or timers that have ms periods. Tradeoff is the maximum timeout encoded with ints. For 32 bits ints should give a maximum of 2147 seconds. 16 bits could be problematic. Perhaps the signature should explicitly have int32_t.

dpgeorge · 2021-11-17T02:17:26Z

@mcdeoliveira thanks for the effort on this and sorry it got left behind. I'll close this PR for now, but feel free to reopen if you're still interested to work on it and take into account the comment #3332 (comment)

…n-main Translations update from Hosted Weblate

Mauricio de Oliveira added 7 commits January 2, 2020 00:55

Support for timed lock acquire. This enables threading functionality.

2cbc0ab

Merge branch 'master' into threads

c1d9f42

# Conflicts: # ports/unix/mpconfigport.h

Merge branch 'master' into threads

e264864

Testing and final touches.

48a80c5

Testing and final touches.

fb726ed

Clean up

bcb3f61

Clean up

9e5b4d4

dlech reviewed Feb 1, 2020

View reviewed changes

Changed from ms to us in timeout

fd623fd

mcdeoliveira commented Feb 2, 2020

View reviewed changes

dpgeorge mentioned this pull request Nov 17, 2021

_thread: timeout parameter is not implemented in Lock.acquire() #3332

Open

dpgeorge closed this Nov 17, 2021

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Nov 19, 2021

Merge pull request micropython#5599 from weblate/weblate-circuitpytho…

4fefb71

…n-main Translations update from Hosted Weblate

dpgeorge mentioned this pull request Jul 20, 2022

py/_thread: Add support for lock.acquire timeout. #8932

Open

DvdGiessen mentioned this pull request May 22, 2024

py/_thread: Add support for lock.acquire timeout. #15099

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timed lock aquisition #5599

Timed lock aquisition #5599

mcdeoliveira commented Feb 1, 2020

dpgeorge commented Feb 1, 2020

dlech left a comment

dlech Feb 1, 2020

dlech Feb 1, 2020 •

edited

Loading

This comment was marked as resolved.

dlech Feb 1, 2020

mcdeoliveira Feb 2, 2020

dlech Feb 1, 2020

mcdeoliveira commented Feb 1, 2020

mcdeoliveira commented Feb 1, 2020

mcdeoliveira commented Feb 2, 2020

mcdeoliveira left a comment

dpgeorge commented Nov 17, 2021

		@@ -0,0 +1,34 @@
		# test timed _thread lock object using a single thread

Timed lock aquisition #5599

Timed lock aquisition #5599

Conversation

mcdeoliveira commented Feb 1, 2020

dpgeorge commented Feb 1, 2020

dlech left a comment

Choose a reason for hiding this comment

dlech Feb 1, 2020

Choose a reason for hiding this comment

dlech Feb 1, 2020 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

dlech Feb 1, 2020

Choose a reason for hiding this comment

mcdeoliveira Feb 2, 2020

Choose a reason for hiding this comment

dlech Feb 1, 2020

Choose a reason for hiding this comment

mcdeoliveira commented Feb 1, 2020

mcdeoliveira commented Feb 1, 2020

mcdeoliveira commented Feb 2, 2020

mcdeoliveira left a comment

Choose a reason for hiding this comment

dpgeorge commented Nov 17, 2021

dlech Feb 1, 2020 •

edited

Loading