Skip to content
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

Add tests for aborted Pool::acquire and Semaphore::acquire calls showing a memory leak #3

Merged
merged 3 commits into from
Jun 11, 2022
Merged

Add tests for aborted Pool::acquire and Semaphore::acquire calls showing a memory leak #3

merged 3 commits into from
Jun 11, 2022

Conversation

bikeshedder
Copy link
Contributor

I wanted to show the reason for the deadlock I reported in #2 but found a memory leak instead.

A aborted Semaphore::acquire does leave the waker behind in the queue which causes the memory to grow indefinitely.

The thing I don't understand: I wanted to show that the aborted task A leaves a waker for itself in the queue and when the permit is returned task A is woken instead of task B thus causing the code to deadlock. This does not happen. Instead I found that calling waker.wake() for task A actually wakes task B.

I'm trying to wrap my head around why it works that way and doesn't deadlock. Whatever the reason for that behavior is. It's a memory leak nonetheless.

@Astro36 Astro36 added the enhancement New feature or request label May 5, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jun 11, 2022

Codecov Report

Merging #3 (26322c6) into main (9702306) will increase coverage by 4.96%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main       #3      +/-   ##
==========================================
+ Coverage   85.14%   90.11%   +4.96%     
==========================================
  Files           6        6              
  Lines         202      263      +61     
==========================================
+ Hits          172      237      +65     
+ Misses         30       26       -4     
Impacted Files Coverage Δ
qp/src/pool.rs 84.54% <100.00%> (+5.02%) ⬆️
qp/src/sync.rs 92.68% <100.00%> (+13.51%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c208a2c...26322c6. Read the comment docs.

@Astro36
Copy link
Owner

Astro36 commented Jun 11, 2022

The thing I don't understand: I wanted to show that the aborted task A leaves a waker for itself in the queue and when the permit is returned task A is woken instead of task B thus causing the code to deadlock. This does not happen. Instead I found that calling waker.wake() for task A actually wakes task B.

I don't know why this happens, but the problem that the queue is not completely empty seems to be caused by calling the Acquire::poll function once more when the Acquire future is cancelled.

This is a commit that just fixed the memory leak issue.

Thank you for your enthusiasm!

@Astro36 Astro36 added the bug Something isn't working label Jun 11, 2022
@Astro36 Astro36 merged commit ff95f6c into Astro36:main Jun 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants