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

Avoid I/O deadlock with conda #581

Merged
merged 2 commits into from
Jan 18, 2024
Merged

Avoid I/O deadlock with conda #581

merged 2 commits into from
Jan 18, 2024

Conversation

tadeu
Copy link
Contributor

@tadeu tadeu commented Jan 15, 2024

Description

conda lock may deadlock with a child conda process, it may be trying to read from stdout here:

for logline in stdout:

while conda is trying to write to stderr (blocked) here:

https://github.com/conda/conda/blob/604f2b7ddf7d8c5a3ef00698fb4bd1ce78eadbd1/conda/exceptions.py#L1258

Reading from stdout in a separate thread allows stderr to be consumed in the main thread.

(It's difficult to write a MRE, but this is happening when conda lock install is calling conda create, right after conda finishes verifying the transaction and is going to report a bunch of ClobberWarnings.)

@tadeu tadeu requested a review from a team as a code owner January 15, 2024 20:59
Copy link

netlify bot commented Jan 15, 2024

Deploy Preview for conda-lock ready!

Name Link
🔨 Latest commit 62c44ac
🔍 Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/65a91b5bb5f9fb00089c61d7
😎 Deploy Preview https://deploy-preview-581--conda-lock.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

`conda lock` may deadlock with a child `conda` process, it may be trying
to read from `stdout` while `conda` is trying to write to `stderr`
(blocked)

Reading from `stdout` in a separate thread allows `stderr` to be
consumed in the main thread.
@maresb
Copy link
Contributor

maresb commented Jan 16, 2024

Thanks so much @tadeu for tracking down this tricky deadlock and submitting a fix for it!!! These things take a lot of time and effort to diagnose, so I'm very grateful.

I'm thinking about an alternative fix in #582 where instead of introducing threads we get rid of subprocess.Popen. I haven't done much subprocess/threads stuff lately, so I'd really like to know what you think about that approach.

@tadeu
Copy link
Contributor Author

tadeu commented Jan 16, 2024

It seems that #582 would also fix the deadlock, but would conda create output still be shown at real time, or would it only be shown once the command finishes? Because, if it's the latter, sometimes conda may take a lot of time to finish.

@tadeu
Copy link
Contributor Author

tadeu commented Jan 16, 2024

Thanks so much @tadeu for tracking down this tricky deadlock and submitting a fix for it!!! These things take a lot of time and effort to diagnose, so I'm very grateful.

And thank you for working on this awesome tool and maintaining it! 😃

@nicoddemus
Copy link
Contributor

It seems that #582 would also fix the deadlock, but would conda create output still be shown at real time, or would it only be shown once the command finishes? Because, if it's the latter, sometimes conda may take a lot of time to finish.

Indeed IIUC that approach would only provide any output at the end of the call, which can take minutes in some cases, so while simpler I think we will lose UX.

The solution in this PR solves the problem (which is really annoying in CI, happens a few times during the day), is simple, and localized to a small part of the code.

Co-authored-by: Bruno Oliveira <bruno@soliv.dev>
@maresb
Copy link
Contributor

maresb commented Jan 18, 2024

Thanks @tadeu, and thanks also @nicoddemus for the much appreciated review!

@maresb maresb merged commit 555046d into conda:main Jan 18, 2024
10 checks passed
@nicoddemus
Copy link
Contributor

Thanks @maresb for merging the fix!

Are there any plans for a new release soon?

@maresb
Copy link
Contributor

maresb commented Jan 18, 2024

Ya, I'm trying to get something out now.

The solution in this PR solves the problem (which is really annoying in CI, happens a few times during the day), is simple, and localized to a small part of the code.

Has this been happening for a while, or is this due to a regression?

@nicoddemus
Copy link
Contributor

Ya, I'm trying to get something out now.

Awesome, many thanks! 🙇

Has this been happening for a while, or is this due to a regression?

Can't say, we just started using conda-lock at 2.5.1.

tadeu added a commit to tadeu/conda-lock that referenced this pull request Jan 18, 2024
maresb added a commit that referenced this pull request Feb 11, 2024
Add explanation and minor improvement to #581
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.

3 participants