Replace `open4` library by `open3` #825

voxik · 2023-07-04T10:56:22Z

for the following reasons:

open4 is 3rd party library while open3 is part of StdLib.
It is not clear if popen is maintained upstream or if it is just stable.
There needis to be special handling for IO.popen4 in JRuby while Open3.popen3 should work the same everywhere.

voxik · 2023-07-04T10:58:43Z

Two things to consider.

I am not sure if the open3 dependency should be explicitly mentioned in the Gemfile, because open3 is part of StdLib. However, I am typically in favor of stating dependencies no matter where they comes from.
There were some exceptions for JRuby, but it does not seem JRuby is covered by test matrix. Not sure why.

Just FYI this conflicts with #824 if I am nit mistaken

voxik · 2023-07-04T11:01:21Z

Hm, not sure about the test errors. It is a pity that the ensure blocks hides the launch_process errors

geemus · 2023-07-06T20:55:46Z

Sounds like pretty good reasons to consider a change.

I also don't recall why jruby isn't in the test matrix, maybe just oversight.

@voxik Could you resolve the conflicts? Thanks!

for the following reasons: 1. `open4` is 3rd party library while `open3` is part of StdLib. 2. It is not clear if `popen` is maintained upstream or if it is just stable. 3. There needis to be special handling for `IO.popen4` in JRuby while `Open3.popen3` should work the same everywhere.

voxik · 2023-07-10T16:26:00Z

I also don't recall why jruby isn't in the test matrix, maybe just oversight.

They were probably failing at some point: 27d23c4

@voxik Could you resolve the conflicts? Thanks!

Done. This time the CI passes. However, It would probably deserve several test rounds, judging by the previous failures.

It was previously dropped by 27d23c4 for too many failures.

voxik · 2023-07-10T16:28:53Z

I also don't recall why jruby isn't in the test matrix, maybe just oversight.

They were probably failing at some point: 27d23c4

Trying to re-enabling the JRuby test suite. Lets see.

voxik · 2023-07-10T16:31:54Z

Hm, failures (not due to JRuby). It might not be that easy after all 😢

geemus · 2023-07-13T15:07:10Z

Yeah. I'm certainly open to being on stdlib, but I also feel like we did it this way for reasons (which are now lost to my memory). So maybe this is a reflection of some of those things.

voxik · 2023-07-14T13:01:11Z

These are two variants of the failures:

  Excon streaming basics
    http
      + returns true
      simple blocking request on streaming endpoint + returns ["Hellostreamyworld", "response time ok"]
      simple blocking request on streaming endpoint with fixed length + returns ["Hellostreamyworld", "response time ok"]
      simple request with response_block on streaming endpoint + returns [["Hello", "streamy", "world"], "response times ok"]
      simple request with response_block on streaming endpoint with fixed length + returns [["Hello", "streamy", "world"], "response times ok"]
    https
      Address already in use - bind(2) for nil port 9292 (Errno::EADDRINUSE)
        tests/test_helper.rb:408:in `initialize'
        tests/test_helper.rb:408:in `new'
        tests/test_helper.rb:408:in `with_ssl_streaming'
        tests/basic_tests.rb:34:in `block (2 levels) in <top (required)>'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.0.0/gems/shindo-0.3.10/lib/shindo.rb:79:in `instance_eval'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.0.0/gems/shindo-0.3.10/lib/shindo.rb:79:in `tests'
        tests/basic_tests.rb:33:in `block in <top (required)>'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.0.0/gems/shindo-0.3.10/lib/shindo.rb:79:in `instance_eval'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.0.0/gems/shindo-0.3.10/lib/shindo.rb:79:in `tests'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.0.0/gems/shindo-0.3.10/lib/shindo.rb:38:in `initialize'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.0.0/gems/shindo-0.3.10/lib/shindo.rb:13:in `new'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.0.0/gems/shindo-0.3.10/lib/shindo.rb:13:in `tests'
        tests/basic_tests.rb:26:in `<top (required)>'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.0.0/gems/shindo-0.3.10/lib/shindo/bin.rb:61:in `load'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.0.0/gems/shindo-0.3.10/lib/shindo/bin.rb:61:in `block (2 levels) in run_in_thread'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.0.0/gems/shindo-0.3.10/lib/shindo/bin.rb:58:in `each'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.0.0/gems/shindo-0.3.10/lib/shindo/bin.rb:58:in `block in run_in_thread'

or

  Excon streaming basics
    http
      + returns true
      simple blocking request on streaming endpoint + returns ["Hellostreamyworld", "response time ok"]
      simple blocking request on streaming endpoint with fixed length + returns ["Hellostreamyworld", "response time ok"]
      simple request with response_block on streaming endpoint + returns [["Hello", "streamy", "world"], "response times ok"]
      simple request with response_block on streaming endpoint with fixed length + returns [["Hello", "streamy", "world"], "response times ok"]
      No child processes (Errno::ECHILD)
        tests/test_helper.rb:327:in `wait'
        tests/test_helper.rb:327:in `cleanup_process'
        tests/test_helper.rb:376:in `with_unicorn'
        tests/basic_tests.rb:29:in `block (2 levels) in <top (required)>'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.1.0/gems/shindo-0.3.10/lib/shindo.rb:79:in `instance_eval'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.1.0/gems/shindo-0.3.10/lib/shindo.rb:79:in `tests'
        tests/basic_tests.rb:27:in `block in <top (required)>'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.1.0/gems/shindo-0.3.10/lib/shindo.rb:79:in `instance_eval'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.1.0/gems/shindo-0.3.10/lib/shindo.rb:79:in `tests'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.1.0/gems/shindo-0.3.10/lib/shindo.rb:38:in `initialize'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.1.0/gems/shindo-0.3.10/lib/shindo.rb:13:in `new'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.1.0/gems/shindo-0.3.10/lib/shindo.rb:13:in `tests'
        tests/basic_tests.rb:26:in `<top (required)>'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.1.0/gems/shindo-0.3.10/lib/shindo/bin.rb:61:in `load'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.1.0/gems/shindo-0.3.10/lib/shindo/bin.rb:61:in `block (2 levels) in run_in_thread'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.1.0/gems/shindo-0.3.10/lib/shindo/bin.rb:58:in `each'
        /home/runner/work/excon/excon/vendor/bundle/ruby/3.1.0/gems/shindo-0.3.10/lib/shindo/bin.rb:58:in `block in run_in_thread'
    https
      + returns true
      simple blocking request on streaming endpoint + returns ["Hellostreamyworld", "response time ok"]
      simple blocking request on streaming endpoint with fixed length + returns ["Hellostreamyworld", "response time ok"]
      simple request with response_block on streaming endpoint + returns [["Hello", "streamy", "world"], "response times ok"]
      simple request with response_block on streaming endpoint with fixed length + returns [["Hello", "streamy", "world"], "response times ok"]

Is there a chance that this is actually issue with Unicorn crashing or getting stuck somehow? Interestingly, I have not hit the issue on my laptop yet.

Actually now I have noticed that locally, I have applied the test on top of 0.97.0 🤔

geemus · 2023-07-14T21:07:57Z

Interesting. Yeah, I could see it being a unicorn issue of some sort.

Similarly for me, checking this branch out locally and running seems to work fine locally.

geemus · 2023-07-14T21:08:28Z

Or at least I see some other noise and maybe questionable things, but no actual errors.

voxik · 2023-07-14T21:23:47Z

I haver run the test suite 100 times in a loop without issues. It might be also some GH CI issue, running multiple test suites in parallel on single machine, where one process occupies specific port? Not sure how the GH CI works ....

voxik · 2023-07-17T10:25:57Z

I wonder how to restart the CI?

geemus · 2023-07-17T14:42:01Z

If you go to the actions page (clicking details next to one of them, for instance), you can scroll to the top and choose to re-run jobs. Is that what you mean?

voxik · 2023-07-17T15:21:43Z

If you go to the actions page (clicking details next to one of them, for instance), you can scroll to the top and choose to re-run jobs. Is that what you mean?

I probably can't do that for the PR, but I can run the jobs for my fork. Thx 👍

geemus · 2023-07-17T15:28:31Z

@voxik Ah, right. Yeah maybe permissions issues there. I think on your fork should be pretty comparable though (hopefully).

voxik · 2023-07-18T10:00:27Z

Playing with the test suite in my repository, I don't think these are new issues. Here are my test runs:

https://github.com/voxik/excon/actions/runs/5574434549

I have mistakenly executed them against the current master (50566a9) and there is the Address already in use - bind(2) for nil port 9292 (Errno::EADDRINUSE) error for latest Ruby 2.7 run

geemus · 2023-07-19T16:04:09Z

Ah, yeah thanks for that heads up.

github-actions · 2023-09-18T01:45:56Z

This pr has been marked inactive and will be closed if no further activity occurs.

geemus · 2023-09-18T15:21:53Z

Pinned this so it won't close, I think we may want to still circle back, I didn't have a chance and then largely forgot.

voxik force-pushed the popen4-to-open3 branch from 0242e92 to b0663f7 Compare July 10, 2023 16:22

Add JRuby into test matrix.

91af4ab

It was previously dropped by 27d23c4 for too many failures.

github-actions bot added the no-pr-activity label Sep 18, 2023

geemus added the pinned label Sep 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `open4` library by `open3` #825

Replace `open4` library by `open3` #825

voxik commented Jul 4, 2023

voxik commented Jul 4, 2023

voxik commented Jul 4, 2023

geemus commented Jul 6, 2023

voxik commented Jul 10, 2023

voxik commented Jul 10, 2023

voxik commented Jul 10, 2023

geemus commented Jul 13, 2023

voxik commented Jul 14, 2023

geemus commented Jul 14, 2023

geemus commented Jul 14, 2023

voxik commented Jul 14, 2023

voxik commented Jul 17, 2023

geemus commented Jul 17, 2023

voxik commented Jul 17, 2023

geemus commented Jul 17, 2023

voxik commented Jul 18, 2023

geemus commented Jul 19, 2023

github-actions bot commented Sep 18, 2023

geemus commented Sep 18, 2023

Replace open4 library by open3 #825

Are you sure you want to change the base?

Replace open4 library by open3 #825

Conversation

voxik commented Jul 4, 2023

voxik commented Jul 4, 2023

voxik commented Jul 4, 2023

geemus commented Jul 6, 2023

voxik commented Jul 10, 2023

voxik commented Jul 10, 2023

voxik commented Jul 10, 2023

geemus commented Jul 13, 2023

voxik commented Jul 14, 2023

geemus commented Jul 14, 2023

geemus commented Jul 14, 2023

voxik commented Jul 14, 2023

voxik commented Jul 17, 2023

geemus commented Jul 17, 2023

voxik commented Jul 17, 2023

geemus commented Jul 17, 2023

voxik commented Jul 18, 2023

geemus commented Jul 19, 2023

github-actions bot commented Sep 18, 2023

geemus commented Sep 18, 2023

Replace `open4` library by `open3` #825

Replace `open4` library by `open3` #825