conda / conda-lock

Lightweight lockfile for conda environments
https://conda.github.io/conda-lock/
Other
456 stars 101 forks source link

Avoid I/O deadlock with conda #581

Closed tadeu closed 5 months ago

tadeu commented 5 months ago

Description

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

https://github.com/conda/conda-lock/blob/aa491499ad6e4fe036bc1be9ae1eec2794002143/conda_lock/invoke_conda.py#L143

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.)

netlify[bot] commented 5 months ago

Deploy Preview for conda-lock ready!

Name Link
Latest commit 62c44ac9caec33ceb99b900842a417aefe3b534c
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.

maresb commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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! :smiley:

nicoddemus commented 5 months ago

It seems that https://github.com/conda/conda-lock/pull/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.

maresb commented 5 months ago

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

nicoddemus commented 5 months ago

Thanks @maresb for merging the fix!

Are there any plans for a new release soon?

maresb commented 5 months ago

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 commented 5 months ago

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

Awesome, many thanks! :bow:

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.