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

Guard against concurrent re-imports. #1270

Merged
merged 4 commits into from
Mar 15, 2021

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented Mar 15, 2021

Although there is currently no identified way this can happen, this fix
is known to work for one victim of #1272.

Fixes #1272

@jsirois
Copy link
Member Author

jsirois commented Mar 15, 2021

This can be tested in Pants via:

  1. Run tox -epackage against this PR. You'll get output like:
    $ tox -epackage
    package recreate: /home/jsirois/dev/pantsbuild/jsirois-pex/.tox/package
    package installdeps: pytoml
    package installed: pytoml==0.1.21
    package run-test-pre: PYTHONHASHSEED='2729908053'
    package run-test: commands[0] | python scripts/package.py
    Building Pex PEX to `dist/pex` ...
    Built Pex PEX @ v2.1.33-2-g14e4aa6:
    sha256: 237a6d2bbb1af80ac642067d954b4c535cada8295a18437a046526f4b1f2c63f
      size: 3596832
    __________________________________________________________________________________________________________________________________________ summary ___________________________________________________________________________________________________________________________________________
      package: commands succeeded
      congratulations :)
    
  2. Execute pants with some extra configuration:
    $ ./pants \
      --download-pex-bin-url-template=file:///home/jsirois/dev/pantsbuild/jsirois-pex/dist/pex \
      --download-pex-bin-known-versions='["v2.1.33|linux|237a6d2bbb1af80ac642067d954b4c535cada8295a18437a046526f4b1f2c63f|3596832"]' \
      ...
    

In step 2 you'd use darwin instead of linux in the --download-pex-bin-known-versions flag value on macOS.

I've attached the pex.zip file if you want to use the one I built using step 1. I had to rename it to pex.zip to work with the GitHub UI and you'll have to rename after download to not have a .zip extension (Pants tries to magically unzip it if it has that extension). Adjust the local file:// URL in the --download-pex-bin-url-template example above to fit your machine and you're off and testing with the patched Pex PEX.

@jsirois jsirois requested a review from tdyas March 15, 2021 15:01
@tdyas
Copy link
Contributor

tdyas commented Mar 15, 2021

Using the pex produced from this PR appears to fix the issue for me. I ran the failing pants command to show the failure, then ran it with this pex tool configured and it went away. Then going back to the regularly-configured pex brings back the failure.

pex/bootstrap.py Show resolved Hide resolved
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thank you for getting to the bottom of this!

@jsirois
Copy link
Member Author

jsirois commented Mar 15, 2021

Thank you for getting to the bottom of this!

You're welcome, but the characterization is incorrect. This fix was the only one I could imagine given all reports / OSes / interpreters / logs, but it still has a logical hole. The fix only works if pex is imported from a background thread while a PEX is bootstrapping. By inspection, this isn't happening. By experiment, this also isn't happening. I instrumented imports with a sys.path_importer_hook and found exactly 1 import from the execute_parallel background thread. That was of encodings.ascii by the Python stdlib message formatting system. That code does do a dynamic __import__. So, without the ability to repro and thus track down the background import, I cannot point to the background import cause.

@jsirois jsirois mentioned this pull request Mar 15, 2021
4 tasks
@jsirois jsirois merged commit a25f1fe into pex-tool:master Mar 15, 2021
@jsirois jsirois deleted the pants-issues/11211 branch March 15, 2021 23:05
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.

The Pex CLI can fail to find pex.bin.
3 participants