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

Fix: resolve deadlock in LatestOsVersion#version_for_os #20329

Merged
merged 1 commit into from Apr 6, 2023

Conversation

stbix
Copy link
Contributor

@stbix stbix commented May 26, 2022

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

When I upgraded to Xcode 13.3.0, fastlane started to deadlock
randomly in LatestOsVersion#version_for_os. Eventually I figured
that it blocks only, when my phone is connected via USB to my
computer. I can "solve" the problem by disconnecting my phone.

Description

LatestOsVersion#version_for_os can be catched in a deadlock when
used with Xcode 13.3.0 or later while a device is physically connected
to the computer.

In this case, xcodebuild will return a few lines of errors on stderr
which will trigger a deadlock in Open3#open3. The problem is
mentioned explicitely in the documentation of Open3#open3:

You should be careful to avoid deadlocks. Since pipes are fixed
length buffer, #popen3(“prog”) {|i, o, e, t| o.read } deadlocks if
the program generates many output on stderr. You should be read
stdout and stderr simultaneously (using thread or IO.select). However
if you don't need stderr output, #popen2 can be used. If merged
stdout and stderr output is not a problem, you can use #popen2e.
If you really needs stdout and stderr output as separate strings,
you can consider #capture3.

Using Open3#capture2 or Open3#open2 would show the error in the
terminal, but Open3#capture3 allows us to capture only stdout and
hide stderr from the users eyes.

I tested the fix using Xcode 13.3.1 connecting and disconnecting
my phone, and it work just fine now.

@google-cla
Copy link

google-cla bot commented May 26, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@sleepygarden
Copy link

sleepygarden commented May 27, 2022

@stbix thank you for this. I don't have connected USB devices, I'm using an amazon ec2 Mac Intel box for our CI pipeline. After upgrading from xcode 13.2.1 to 13.4 this issue kicked in for me as well.

@sleepygarden
Copy link

bump on this - I'd love to see this get merged, my team has been unable to upgrade fastlane for over half a year because of this

@mbarany
Copy link

mbarany commented Mar 24, 2023

Bump ^

@mbarany
Copy link

mbarany commented Mar 31, 2023

@giginet 🙏 please

Copy link
Collaborator

@giginet giginet left a comment

Choose a reason for hiding this comment

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

Ruby's standard libraries are too difficult...

LGTM. Sorry for waiting such a long time.

LatestOsVersion#version_for_os can be catched in a deadlock when
used with Xcode 13.3.0 or later while a device is physically connected
to the computer.

In this case, xcodebuild will return a few lines of errors on stderr
which will trigger a deadlock in Open3#open3.  The problem is
mentioned explicitely in the documentation of Open3#open3:

> You should be careful to avoid deadlocks. Since pipes are fixed
> length buffer, #popen3(“prog”) {|i, o, e, t| o.read } deadlocks if
> the program generates many output on stderr. You should be read
> stdout and stderr simultaneously (using thread or IO.select). However
> if you don't need stderr output, #popen2 can be used. If merged
> stdout and stderr output is not a problem, you can use #popen2e.
> If you really needs stdout and stderr output as separate strings,
> you can consider #capture3.

Using Open3#capture2 or Open3#open2 would show the error in the
terminal, but Open3#capture3 allows us to capture only stdout and
hide stderr from the users eyes.
@giginet giginet force-pushed the fix-deadlock-in-latest-os-version branch from 3fe8787 to 8733889 Compare April 6, 2023 12:55
@giginet
Copy link
Collaborator

giginet commented Apr 6, 2023

@stbix Sorry for waiting for you such a long time.

Your commit looks good.
However, it passed a too long time, so I rebased your commit to the current HEAD of the master branch to re-run CI jobs because you were allowed to edit this PR. 🙏

I'll merge this soon after CI passed. Thank you for your contribution!

@giginet giginet merged commit 0c06ab0 into fastlane:master Apr 6, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants