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

Use IPC::Run3::run3 rather than IPC::Open3::open3 #103

Closed
wants to merge 1 commit into from
Closed

Use IPC::Run3::run3 rather than IPC::Open3::open3 #103

wants to merge 1 commit into from

Conversation

aeruder
Copy link

@aeruder aeruder commented Jun 16, 2018

With IPC::Open3::open3 we can create deadlocks in two different situations:

1.) Git command writes more than a pipe buffer of data to
stdout/stderr before reading stdin.

2.) Git command writes more than a pipe buffer of data to stderr
before stdout.

Solving this with Open3 involves dealing with select and a bunch of other
messiness - let's just use IPC::Run3 which solves this problem already.

Fixes issue #100

With IPC::Open3::open3 we can create deadlocks in two different situations:

1.) Git command writes more than a pipe buffer of data to
stdout/stderr before reading stdin.

2.) Git command writes more than a pipe buffer of data to stderr
before stdout.

Solving this with Open3 involves dealing with select and a bunch of other
messiness - let's just use IPC::Run3 which solves this problem already.

Fixes issue #100
@coveralls
Copy link

Coverage Status

Coverage increased (+2.3%) to 87.083% when pulling 21c70d3 on ZipRecruiter:aeruder-fix-open3-deadlock into c616247 on genehack:master.

@genehack
Copy link
Owner

Merged manually; trial release coming soon.

@genehack genehack closed this Jul 17, 2018
@kyzn
Copy link

kyzn commented Feb 28, 2019

Hi @genehack! It would be really great if this patch was released to CPAN. Do you have any plans -- are there any other bugs that you wanted fixed before doing a full release? Just let us know if we can help 🙂
Thanks!

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.

4 participants