Skip to content

Conversation

@kien-truong
Copy link
Contributor

This prevents a deadlock between the main thead and download threads when the threadpool is shutdown prematurely.

Fixes #2032 🦕

@kien-truong kien-truong requested review from a team as code owners September 28, 2024 03:14
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: bigquery Issues related to the googleapis/python-bigquery API. labels Sep 28, 2024
@Linchin Linchin added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 3, 2024
@Linchin Linchin self-assigned this Oct 3, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 3, 2024
@Linchin Linchin removed their assignment Oct 8, 2024
@kien-truong kien-truong force-pushed the fix-bq-storage-client-deadlock branch from 256b0f7 to 3f8bd10 Compare October 10, 2024 15:12
@kien-truong
Copy link
Contributor Author

Just dropping by to see if there's any concerns about this PR.

This prevents a deadlock between the main thead and download threads when the threadpool is shutdown prematurely.
@kien-truong kien-truong force-pushed the fix-bq-storage-client-deadlock branch from d3f2f1e to 8eebb9d Compare November 3, 2024 03:37
@chalmerlowe chalmerlowe added the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 18, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 18, 2024
@chalmerlowe
Copy link
Collaborator

I took a quick look.
Gonna run our CI/CD checks on this.
My first thought is that because we are adding a couple code paths coverage is likely to fail.
Also curious why the pragma for queue.full: we recently got bitten by having a code branch that was not tested and I am hesitant about adding more such paths to the code base without understanding for myself why it is necessary/desireable.

@kien-truong
Copy link
Contributor Author

The queue.Full code path is hard to test because it's non-deterministic: the queue can be full or not depending on the timing of consumers and producers.

So, I just put a pragma there to ignore the coverage, similar to the existing code a few lines below.

except queue.Empty: # pragma: NO COVER

@tswast tswast added the kokoro:run Add this label to force Kokoro to re-run the tests. label Feb 3, 2025
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Feb 3, 2025
@tswast
Copy link
Contributor

tswast commented Feb 3, 2025

Looks like cover is passing.

@tswast tswast merged commit 54c8d07 into googleapis:main Feb 3, 2025
16 of 22 checks passed
@kien-truong kien-truong deleted the fix-bq-storage-client-deadlock branch February 4, 2025 02:18
@tswast
Copy link
Contributor

tswast commented Feb 4, 2025

After sleeping on this, I do think it's important to try and cover the currently uncovered branch. I'll see what I can do in a follow-up PR today.

Edit: mailed #2127

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: bigquery Issues related to the googleapis/python-bigquery API. size: s Pull request size is small.

7 participants