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

JRuby - Add Puma::MiniSSL::Engine#init? and #teardown methods, run all SSL tests #2317

Merged
merged 2 commits into from Sep 1, 2020

Conversation

MSP-Greg
Copy link
Member

Description

JRuby implementation has always lacked Puma::MiniSSL::Engine#init? and #teardown methods, which are needed for use with SSL. Added methods and set all SSL tests to run with JRuby.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@MSP-Greg
Copy link
Member Author

@eregon

Please see Ubuntu TruffleRuby failure at:
https://github.com/puma/puma/pull/2317/checks?check_run_id=896563842#step:7:579

The macOS TruffleRuby job passed.

@eregon
Copy link
Contributor

eregon commented Jul 22, 2020

Sorry about that, it looks like very weird transient, where it fails to unlock a lock it locked right above. So it's rather puzzling.
Could you rerun it?

@MSP-Greg
Copy link
Member Author

@eregon

Sorry about that... Could you rerun it?

No problem, the tests do have intermittent failure issues, some of which may be caused by parallel testing.

I ran it it my fork with the same seed, and it passed:

https://github.com/MSP-Greg/puma/runs/898621881

@MSP-Greg
Copy link
Member Author

MSP-Greg commented Jul 22, 2020

Small revision to better align MRI & JRuby SSL code.

@MSP-Greg
Copy link
Member Author

MSP-Greg commented Jul 22, 2020

@JohnPhillips31416

Re the discussion in your PR #2302 (which should be merged soon), I've tried to add Puma::MiniSSL::Engine#init? and Puma::MiniSSL::Engine#shutdown in a manner that is similar to the MRI code. I also enabled all the SSL tests for JRuby.

It's passing here. If you have time, could you have a look at it? Some of the logging is different, I need to look a that some more, but some may also be due to differences in how Nio4r handles TCP/SSL/Unix sockets...

@JohnPhillips31416
Copy link
Contributor

Looks fine, I don't see any issues.

@nateberkopec nateberkopec added maintenance waiting-for-review Waiting on review from anyone labels Jul 31, 2020
@dentarg
Copy link
Member

dentarg commented Sep 1, 2020

@MSP-Greg can you rebase?

@nateberkopec what about merging in this one? should make us not see

Error in reactor loop escaped: undefined method `init?' for #<Puma::MiniSSL::Engine:0x3fffddf3> (NoMethodError)

in the output from the jruby tests, which would be nice (example)

Add Puma::MiniSSL::Engine#init? and #teardown methods
@MSP-Greg MSP-Greg marked this pull request as ready for review September 1, 2020 21:43
@MSP-Greg MSP-Greg merged commit fa6e916 into puma:master Sep 1, 2020
@MSP-Greg MSP-Greg deleted the jruby-ssl branch September 1, 2020 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance waiting-for-review Waiting on review from anyone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants