Skip to content

Acquire Lock Before Releasing Python GIL#386

Merged
christian-rauch merged 1 commit intoAprilRobotics:masterfrom
Jgunde:master
Jun 11, 2025
Merged

Acquire Lock Before Releasing Python GIL#386
christian-rauch merged 1 commit intoAprilRobotics:masterfrom
Jgunde:master

Conversation

@Jgunde
Copy link
Contributor

@Jgunde Jgunde commented Jun 1, 2025

This PR implements a lock in apriltag_pywrap.c to prevent multiple threads which share the same detector object from calling the detect function, apriltag_detector_detect(), at the same time.

Previously, the Python GIL was released during the call to apriltag_detector_detect(). This would cause the program to crash if multiple threads called this function at the same time. By implementing a lock, only a single thread can call it at a time.

The added functionality was tested using the following Python code:

from PIL import Image
import threading

import numpy as np
from apriltag import apriltag


image = Image.open("apriltag.png").convert("L")  # Load apriltag image and convert to grayscale.
image_array = np.array(image)

detector = apriltag(
    family="tag36h11",
    threads=4,
    maxhamming=1,
    decimate=1.0,
    blur=0.0,
    refine_edges=True,
    debug=False
)


def worker(thread_id):
    for i in range(100):
        detector.detect(image_array)
    print(f"Thread {thread_id} complete.")


# Create and start multiple threads
threads = []
for i in range(10):
    thread = threading.Thread(target=worker, args=(i,))
    thread.start()
    threads.append(thread)

# Wait for all threads to finish
for thread in threads:
    thread.join()

Copy link
Collaborator

@christian-rauch christian-rauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for taking care of the issue.

@christian-rauch
Copy link
Collaborator

Unfortunately, the signing key for the ROS apt repo expired yesterday and hasn't been updated in the ros-tooling/setup-ros action yet. The CI will not pass until this has been resolved.

@Jgunde
Copy link
Contributor Author

Jgunde commented Jun 1, 2025

I added a few changes:

  • Modified comments to improve clarity
  • Initialized detections variable with NULL so it doesn't point to a random address
  • Removed if statement. Since PyThread_acquire_lock(self->det_lock, 1); is blocking, it almost never fails. The contents of the if block was essentially dead code.

Code was tested in the same manner as above.

@christian-rauch
Copy link
Collaborator

The CI seems to work again. Can you rebase and squash your commits together, and then force push again?

@christian-rauch christian-rauch merged commit ce3ccd6 into AprilRobotics:master Jun 11, 2025
51 of 55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants