Add Windows to Actions CI, Ruby 2.5, misc fixes #20

MSP-Greg · 2021-04-28T15:24:37Z

No description provided.

rhenium · 2021-04-28T16:02:17Z

Delete Gemfile.lock

Should this commit add it to .gitignore too?

MSP-Greg · 2021-04-28T16:04:51Z

@rhenium

Good idea, probably the reason it was added... I'll fix

EDIT: done

rhenium · 2021-04-28T16:09:44Z

test/net/http/test_https.rb

  CA_CERT = OpenSSL::X509::Certificate.new(read_fixture("cacert.pem"))
  SERVER_KEY = OpenSSL::PKey.read(read_fixture("server.key"))
  SERVER_CERT = OpenSSL::X509::Certificate.new(read_fixture("server.crt"))
  DHPARAMS = OpenSSL::PKey::DH.new(read_fixture("dhparams.pem"))
  TEST_STORE = OpenSSL::X509::Store.new.tap {|s| s.add_cert(CA_CERT) }

  CONFIG = {
-    'host' => '127.0.0.1',


Do you have the error output without this change?

I'm wondering if those rescue SystemCallError lines scattered over the file are relevant to this. If this (bind to "localhost") works, can they be removed safely now?

They may be. Current mingw jobs in ruby/ruby are failing because of this (see https://github.com/ruby/ruby/actions). I don't have any CI logs in my fork. I fixed locally, then pushed...

I didn't look at any blame/history, but if it's never been tested on Windows...

From https://github.com/ruby/ruby/runs/2456377434?check_suite_focus=true:

1) Failure: TestNetHTTPS#test_session_reuse [D:/a/ruby/ruby/src/test/net/http/utils.rb:48]: <[]> expected but was <["[2021-04-28 10:35:33] ERROR Errno::ECONNABORTED: An established connection was aborted by the software in your host machine.\n" + "\tD:/a/ruby/ruby/build/.ext/common/openssl/buffering.rb:80:in `sysread'\n" + "\tD:/a/ruby/ruby/build/.ext/common/openssl/buffering.rb:80:in `fill_rbuff'\n" + "\tD:/a/ruby/ruby/build/.ext/common/openssl/buffering.rb:323:in `eof?'\n" + "\tD:/a/ruby/ruby/src/tool/lib/webrick/httpserver.rb:82:in `run'\n" + "\tD:/a/ruby/ruby/src/tool/lib/webrick/server.rb:310:in `block in start_thread'\n"]>. 2) Failure: TestNetHTTPS#test_session_reuse [D:/a/ruby/ruby/src/test/net/http/test_https.rb:166]: <true> expected but was <false>.

Thanks for the pointer, but this puzzles me... I was expecting the failing test cases to be test_get_SNI and alike, which were only recently added and didn't have rescue SystemCallError yet. (FWIW, the explanation for rescue is in 2504847.)

I hate this. I pushed the test file change to ruby-loco and my ruby/ruby fork. test-all passed everywhere.

But, in test-spec, it's timing out in ruby-loco, and there are errors in:
spec/ruby/library/net/http/http/active_spec.rb
spec/ruby/library/net/http/http/get2_spec.rb

Maybe more...

Re the failures, thanks for looking further. I can revert that commit and the other rescue SystemCallError code?

I'm looking at the spec failures.

Regarding localhost vs 127.0.0.1 and rescue, I /think/ the changes in this Pull Request are correct, but I can't confirm - I don't know in which environment it is actually needed. Without the changes, TestNetHTTPS#test_min_version must fail in such setup, but we have no such a setup in our CI (rubyci.org / GitHub Actions).

`test_get_SNI` was a wrong example. It does `http.ipaddr = config('host')`, so it was fine as is.

ruby/ruby's Github Actions failure on mingw (TestNetHTTPS#test_session_reuse) looks like a separate issue (as I wrote in my last comment).

@rhenium

Thanks for your additional investigation. I saw ruby-loco failed, then went to ruby/ruby, and saw Ubuntu & macOS passing (along with CI here), but failing mingw, I assumed it was probably a test issue. Didn't even look at lib code here. I did review the code in ruby/openssl test_ssl_session.rb.

Anyway, re the sleep 2.5 addition, I think http.finish is closing the socket, and the handshake occurs when http.start is called (it calls connect_nonblock)? So I'm not sure what the sleep is affecting, as I would think keep_alive_timeout would be different than a 'data' timeout, but I haven't looked...

Also, wondering whether to define HOST_IP = '127.0.0.1' and use it for all refs in the test?

EDIT:
Re test_session_reuse, the following works:

http.start http.get("/") http.finish http.start assert_equal true, http.instance_variable_get(:@socket).io.session_reused? assert_equal $test_net_http_data, http.get("/").body http.finish

#21 should fix TestNetHTTPS#test_session_reuse, though running the test suite takes 10 minutes on Windows. This is due to the slow fallback from IPv6 (localhost) to IPv4 (127.0.0.1) and this (your) Pull Request should resolve it.

Session resumption was not done correctly because the test case was establishing 3 TLS connections in total instead of the intended 2 due to the bug & a change in OpenSSL 1.1.1 which prohibited using a session ticket more than once in TLS 1.3 clients (openssl/openssl#6601).

change in OpenSSL 1.1.1

Thanks. I vaguely recalled something about that, but my finding it might have taken a while...

Should I change test_session_reuse as above? session_reused? is important, but the real test is can the socket be used, and hence, the body assert?

@rhenium

I removed a Rakefile change that wasn't needed. I revised the test as above, and also added the constant. Changed it to an IPv6 address locally, and all passed.

And, since I haven't contributed here previously, it needs approval for CI. Passed in my fork. Thanks.

Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

MSP-Greg · 2021-04-30T17:02:37Z

Since this changes a few files that are also changed in #19, I added Ruby 2.5 to Actions, and listed @deivid-rodriguez as a co-author.

Testing was being run with rake test, changed to bundle exec rake test since bundle install is used.

deivid-rodriguez · 2021-05-01T10:26:27Z

I'm still in favour of releasing a gemified version of net-http still supporting ruby 2.5, and then dropping support, but it sounds unrelated to this PR.

MSP-Greg · 2021-05-01T17:25:16Z

This PR does not change any 'lib' files, only files related to testing and building. Using Ruby 2.5 is part of that.

I'll be happy to revert those changes.

This PR fixes an issue that has happened before, and that is a ruby repo (std-lib, default or bundled gem) breaking CI in ruby/ruby, which has been failing since 27-Apr...

deivid-rodriguez · 2021-05-03T11:59:23Z

Restoring support for ruby 2.5 seems unrelated to getting the CI back to green. The fact that two separate PRs change the same part of a repository doesn't mean they are related or should be introduced together.

That said, I don't make any decisions here, but again, I'd be very happy if ruby 2.5 support is restored. I just feel it unnecessarily makes this PR more controversial 🤷‍♂️.

We'll see what maintainers say.

hsbt · 2024-07-10T23:21:09Z

Fixed at #181

Replacing localhost to HOST and 127.0.0.1 to HOST_IP is seems good. If you still interest that, can you submit another PR? Thanks.

MSP-Greg force-pushed the actions-windows branch from 56fb418 to 93944f5 Compare April 28, 2021 16:07

rhenium reviewed Apr 28, 2021

View reviewed changes

MSP-Greg force-pushed the actions-windows branch from 93944f5 to f6ae5f4 Compare April 28, 2021 17:48

MSP-Greg mentioned this pull request Apr 28, 2021

Fix windows net https issues in test-all and test-spec ruby/ruby#4424

Merged

rhenium approved these changes Apr 29, 2021

View reviewed changes

MSP-Greg force-pushed the actions-windows branch from f6ae5f4 to d1b02d3 Compare April 29, 2021 17:02

MSP-Greg and others added 3 commits April 30, 2021 11:46

Actions - add Windows, Ruby 2.5, bundle exec rake test

1f9c600

Delete Gemfile.lock, add to .gitignore

f8487a5

Fix test_https.rb host, Rakefile

4fbd463

Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

MSP-Greg force-pushed the actions-windows branch from d1b02d3 to 4fbd463 Compare April 30, 2021 16:57

MSP-Greg changed the title ~~Add Windows to Actions CI, misc fixes~~ Add Windows to Actions CI, Ruby 2.5, misc fixes Apr 30, 2021

hsbt closed this Jul 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Windows to Actions CI, Ruby 2.5, misc fixes #20

Add Windows to Actions CI, Ruby 2.5, misc fixes #20

MSP-Greg commented Apr 28, 2021

rhenium commented Apr 28, 2021

MSP-Greg commented Apr 28, 2021 •

edited

Loading

rhenium Apr 28, 2021

MSP-Greg Apr 28, 2021

rhenium Apr 28, 2021

MSP-Greg Apr 28, 2021 •

edited

Loading

MSP-Greg Apr 28, 2021

rhenium Apr 29, 2021

MSP-Greg Apr 29, 2021 •

edited

Loading

rhenium Apr 29, 2021

MSP-Greg Apr 29, 2021

MSP-Greg Apr 29, 2021

MSP-Greg commented Apr 30, 2021

deivid-rodriguez commented May 1, 2021

MSP-Greg commented May 1, 2021

deivid-rodriguez commented May 3, 2021 •

edited

Loading

hsbt commented Jul 10, 2024

Add Windows to Actions CI, Ruby 2.5, misc fixes #20

Add Windows to Actions CI, Ruby 2.5, misc fixes #20

Conversation

MSP-Greg commented Apr 28, 2021

rhenium commented Apr 28, 2021

MSP-Greg commented Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSP-Greg Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSP-Greg Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSP-Greg commented Apr 30, 2021

deivid-rodriguez commented May 1, 2021

MSP-Greg commented May 1, 2021

deivid-rodriguez commented May 3, 2021 • edited Loading

hsbt commented Jul 10, 2024

MSP-Greg commented Apr 28, 2021 •

edited

Loading

MSP-Greg Apr 28, 2021 •

edited

Loading

MSP-Greg Apr 29, 2021 •

edited

Loading

deivid-rodriguez commented May 3, 2021 •

edited

Loading