Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DvdGiessen
Copy link
Contributor

Fixes #3332. Based on previous work in #5599 and #8932.

Differences to previous PR's:

  • Uses a per-port resolution
    • This prevents us from always converting to microseconds and back on ports that don't have that level of precision, no longer limiting the maximum timeout.
    • Also used to implement _thread.TIMEOUT_MAX (see CPython docs), which in turn is used to raise an OverflowError if a too large timeout is given.
  • Maintains the MICROPY_PY_THREAD_LOCK_TIMEOUT feature flag from @mcdeoliveira, since some ports do not support timeouts yet.
    • Adds POSIX feature flags check since apparently my MacOS system doesn't support pthread_mutex_timedlock (see here).
    • This flag also controls the availability of _thread.TIMEOUT_MAX, which is used in the tests to determine whether timeouts are supported or the test should be skipped.
  • Takes into account comments from @dpgeorge in _thread: timeout parameter is not implemented in Lock.acquire() #3332 (comment)
    • Modifies the existing mp_thread_mutex_lock to add full timeout support instead of adding an extra function.
    • Uses int64_t for the timeout value (see note below).

It still has some open issues / questions:

  • The code for adding an constant float/int to the _thread class is wrong. Looking at other examples like math.PI and sys.maxsize, it looks like currently all these are defined in the global obj.h/objint.h so they can have separate implementations for each supported storage format?
    • Even nicer would be if we had some helpers / defines to make it easier to define large integer / float constants, without having to put them into the global int/float implemenation files. But that's probably out of scope for this PR.
  • I've switched to int64_t because on the Unix port the max timeout would otherwise be a mere 35 minutes. However on many other ports the resolution method (see above) already allows for longer timeouts. We could add another per-port define for the type so we could use a smaller type on some ports? I'm not sure if that would be a meaningful improvement for the smaller microcontrollers.
  • The TIMEOUT_MAX value is not entirely accurate; it does for example not take into account the mp_int_t size limit, or on the Unix port the fact that we cannot use the full uint32 range because the current time should be subtracted.

Feedback is welcomed!

Copy link

github-actions bot commented May 22, 2024

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:  +554 +0.065% standard[incl +8(data)]
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:  +232 +0.025% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

@dpgeorge dpgeorge added the py-core Relates to py/ directory in source label Jun 26, 2024
@DvdGiessen DvdGiessen force-pushed the py_thread_lock_timeout branch from 1647e9d to 1f0ca34 Compare August 31, 2024 23:01
@DvdGiessen DvdGiessen force-pushed the py_thread_lock_timeout branch from 1f0ca34 to 55fd6d9 Compare April 16, 2025 12:07
Co-authored-by: Mauricio de Oliveira <mauricio@complexarts.net>
Co-authored-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
Signed-off-by: Daniël van de Giessen <daniel@dvdgiessen.nl>
@DvdGiessen DvdGiessen marked this pull request as ready for review April 16, 2025 12:10
Copy link

codecov bot commented Apr 16, 2025

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.53%. Comparing base (f498a16) to head (55fd6d9).

Files with missing lines Patch % Lines
py/modthread.c 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #15099      +/-   ##
==========================================
- Coverage   98.54%   98.53%   -0.01%     
==========================================
  Files         169      169              
  Lines       21890    21898       +8     
==========================================
+ Hits        21571    21578       +7     
- Misses        319      320       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

1 similar comment
Copy link

codecov bot commented Apr 16, 2025

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.53%. Comparing base (f498a16) to head (55fd6d9).

Files with missing lines Patch % Lines
py/modthread.c 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #15099      +/-   ##
==========================================
- Coverage   98.54%   98.53%   -0.01%     
==========================================
  Files         169      169              
  Lines       21890    21898       +8     
==========================================
+ Hits        21571    21578       +7     
- Misses        319      320       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
py-core Relates to py/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

_thread: timeout parameter is not implemented in Lock.acquire()
2 participants