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 Support #4

Open
dgolombek opened this issue Mar 18, 2018 · 8 comments
Open

JRuby Support #4

dgolombek opened this issue Mar 18, 2018 · 8 comments

Comments

@dgolombek
Copy link

The oj gem is MRI specific. Will you accept a patch to convert this gem to use multi_json so that it picks a JRuby friendly JSON library when appropriate? It defaults to Oj, when that gem is available. This will also allow users who've standardized on some other JSON library to continue using it, without pulling in another JSON gem.

@haotianw465
Copy link
Contributor

Thank you for your feedback. As long as the patch doesn't break MRI it should be fine. However, the SDK is not tested under JRuby, especially a few places the SDK is not tested to be thread-safe under JRuby due to the lack of GIL.

You are welcome to provide PRs or feedback on JRuby support so we can prioritize our work accordingly.

@dgolombek
Copy link
Author

Can you tell me about the ruby 2.3.6 version requirement? JRuby is 2.3.X compatible, but currently hardcodes RUBY_VERSION as 2.3.3.

I think I have everything else working now, and will be ready to push up once the version issue is dealt with.

@haotianw465
Copy link
Contributor

Ruby 2.3.6 has a few security fixes https://www.ruby-lang.org/en/news/2017/12/14/ruby-2-3-6-released/. We generally don't support versions that have known security vulnerabilities.

Do you have issue installing the SDK as a dependency because of this?

@dgolombek
Copy link
Author

Unfortunately there's no way to have parts of the gemspec be conditional on ruby interpreter, so right now it's a hard blocker. I can ask the JRuby team about bumping that version number a tiny bit higher, but I suspect they just want to wait until JRuby 9.2.0.0 is released, with Ruby 2.5.X compatibility. So yes, for now it's a hard blocker for JRuby.

@haotianw465
Copy link
Contributor

I'm sorry to hear that. Our beta version SDK is tested under 2.3, 2.4 and 2.5 with only MRI. For other interperters we would like to hear some feedback from users to decide the priority to support them.

For JRuby based on the discussion I think there are three main issues:

  1. RUBY_VERSION not compatible with JRuby 9.1.x
  2. Oj is MRI specific
  3. The SDK is not tested when GIL is not present.

From the JRuby website "JRuby 9.1.x is our current major version of JRuby. It is expected to be compatible with Ruby 2.3.x and stay in sync with C Ruby." If C Ruby has security fixes I don't see a reason they want to still hard code an older minor version.

I will check with JRuby on what is the latest. But please feel free to update what you find with them as well. Then we can discuss the next step.

@dgolombek
Copy link
Author

Ok, I checked with JRuby team and they suggested that JRuby 9.2 (with MRI 2.5 compatibility) is being worked on for a end of April release and doing the minor version bump was more effort than it was worth before then (since there may not be another release prior to 9.2).

I'll put together the rest of changes now and we'll get those in, then when JRuby 9.2 comes out, I'll do further testing on the thread-safety piece. One of the services I'll be incorporating this into has hundreds of threads active most of the time, so it'll get a workout.

dgolombek pushed a commit to dgolombek/aws-xray-sdk-ruby that referenced this issue Mar 22, 2018
This is part of aws#4. Until JRuby 9.2.0.0 is released, the
required_ruby_version in the gemspec will keep this from working there,
as well as jruby/jruby#5099.

* Replaced Oj with MultiJson + Oj OR JrJackson
* Removed Oj.dump parameters, since those are defaults when using MultiJson
* Remove rdiscount for Markdown generation, use default Yard generator.
* Made two minor code syntax changes to make the Yard generator happy

I've roughly compared the generated RDoc and they look very similar.
@dgolombek
Copy link
Author

I finally have a chance to get back and work on this some more, now that JRuby 9.2.0.0 has been released. However, something odd happened with the 0.10.2 release that was pushed to RubyGems -- it still requires the oj gem (despite having removed that from the gemspec). I can't reproduce this locally by building the gem, so I'm wondering if perhaps the gem was built in an unclean environment?

Separately, do you have particular places of concern for thread safety? Some of the lazily allocated attrs in Entity are problematic, as are the Metadata namespaces (depending upon how they're used). I haven't thought through all the usecases yet, but the TLS-based Contexts are a good start to limit threading issues.

@haotianw465
Copy link
Contributor

It looks like a miss on our end. The 0.10.2 release doesn't have oj related commit. Sorry for the confusion. We will roll it out soon.

This gem went through thread safety testing under MRI. It's usually the places where multiple threads adding/streaming subsegments with the same parent (which is owned by their parent thread) that might cause problems. To officially announce support for JRuby we will need to do thorough testing and potentially add locks in a few places.

JRuby support is on our backlog but we can't guarantee an ETA. You are welcome to submit a PR and we are happy to work with you to get this support out.

@haotianw465 haotianw465 changed the title Gem is not JRuby compatible due to use of Oj gem JRuby Support Oct 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants