Replace thread_safe with concurrent-ruby

[junaruga]

Because thread_safe is deprecated, as it was merged to concurrent-ruby.

https://github.com/ruby-concurrency/thread_safe
[DEPRECATED] Thread-safe collections for Ruby (merged with concurrent-ruby)

https://github.com/ruby-concurrency/concurrent-ruby
Collection classes that were originally part of the (deprecated) thread_safe gem:
...
Hash A thread-safe subclass of Ruby's standard Hash.
...

[junaruga]

The Travis test was failed for only Ruby 1.8.
https://travis-ci.org/asciidoctor/asciidoctor/builds/408586518

Because concurrent-ruby latest version 1.0.5 requires Ruby >= 1.9.3.

To run concurrent-ruby on Ruby 1.8, we have to use the old version 0.5.0.
https://rubygems.org/gems/concurrent-ruby/versions/0.5.0
However the version 0.5.0 does not have Concurrent::Hash class. it might be implemented at different class.

How do you think to implement to fix it?
Do you like to implement to use thread_safe only if Ruby < 1.9.3 ?

[mojavelinux]

Thanks for raising this issue.

Since we're not technically supporting Ruby 1.8.7 anymore, and this is just for tests, you can add an exception in the Gemfile.

Find:

if ruby_version < (Gem::Version.new '1.9.3')

and add:

gem 'thread_safe', '~> 0.3.0'

[mojavelinux]

Can you also add an entry to the CHANGELOG.adoc when you update the PR? Please use the category "Build / Infrastructure". Thanks!

[junaruga]

this is just for tests

thread_safe is not only for test. It is implemented.
Adding gem 'thread_safe', '~> 0.3.0' to Gemfile is meaningless, if we do not implement the conditional requiring thread_safe logic here.

[mojavelinux]

Aha! I see what you are saying. Let me think about it. We might be able to put stubs in place for Ruby 1.8.7 or we can just make this part of the 2.0.0 release.

[junaruga]

Okay, I will wait you :)

[junaruga]

Since we're not technically supporting Ruby 1.8.7 anymore, and this is just for tests, you can add an exception in the Gemfile.

Possible solution 1:

Add only concurrent-ruby to Gemfile, removing thread_safe from asciidoctor.gemspec.
As the test for concurrent-ruby is skipped on Ruby 1.8 when cucorrent-ruby is not installed, we can keep the Ruby 1.8 test case on Travis.

group :development do
...
  if ruby_version < (Gem::Version.new '1.9.3')
  ...
    else
..
      s.add_development_dependency 'concurrent-ruby', '~> 1.0.5'
    end

Possible solution 2:

Add both concurrent-ruby and thread_safe to Gemfile, removing thread_safe from asciidoctor.gemspec.

In this solution, we need implementation to use concurrent-ruby on Ruby >= 1.9.3, and thread_safe on only Ruby < 1.9.3.

Possible solution 3:

Just remove Ruby 1.8 test case from Travis, keeping s.add_development_dependency 'concurrent-ruby', '~> 1.0.5' in asciidoctor.gemspec.

[junaruga]

Can you also add an entry to the CHANGELOG.adoc when you update the PR? Please use the category "Build / Infrastructure". Thanks!

I can update the CHANGELOG.adoc. But is it the category "Build / Infrastructure"?
I replaced thread_safe used in the code at lib/asciidoctor/converter/factory.rb to concurrent in this PR.

[junaruga]

By the way, though it's not related to this PR, seeing current Gemfile,

if ruby_version < (Gem::Version.new '1.9.3')

This kind of brackets style (Gem::Version.new 'X.Y.Z') is used in the file.
But maybe I think that Gem::Version.new('X.Y.Z') style is more popular like below code.

https://github.com/rubygems/rubygems/blob/v2.7.7/lib/rubygems/specification.rb#L2570
if Gem::Version.new(Gem::VERSION) >= Gem::Version.new('1.2.0') then

Just I would share you the information :)

[mojavelinux]

I'm very strongly in favor of lisp-style brackets when writing Ruby. I use it everywhere. I've documented this as part of the style for Asciidoctor projects here: https://github.com/asciidoctor/jekyll-asciidoc/blob/master/coding-style-guide.adoc. That will get moved into the documentation once we roll out 2.0.

[junaruga]

Okay, it is great for this project to have a coding style guide.

[mojavelinux]

I've proposed a revision which maps Concurrent::Hash to ThreadSafe::Cache in Ruby 1.8.7 so tests will pass. We really aren't worrying about Ruby 1.8.7 at this point, so as long as the CI job runs, it's fine.

[mojavelinux]

I recognize this solution is not pretty, but it's just temporary until Asciidoctor 2.0.0, when we'll be dropping support for all these old Ruby versions.

[junaruga]

All right. I like the solution. The effort to support Ruby 1.8 until Asciidoctor 2.0.0.

By the way, you pushed your commit to my repository to add your commit in this PR.
I did not know that was possible. Did I give you the permission in the past time?
https://github.com/junaruga/asciidoctor/commits/feature/replace_thread-safe-to-concurrent-ruby

Anyway, thank you for the working.

[junaruga]

This part should be changed? This PR is not only for Build / Infrastructure?

[mojavelinux]

Since it changes the APIs used by this project, you're right. It's not just a dependency change. Therefore, it should now be under Improvements.

[junaruga]

Okay, I moved it to Improvement.

[mojavelinux]

Thanks!

[junaruga]

Should I move my comment to another category in CHANGELOG.adoc?

[mojavelinux]

By the way, you pushed your commit to my repository to add your commit in this PR.
I did not know that was possible. Did I give you the permission in the past time?

It's a rather recent feature of GitHub and it's the default setting when contributing to this project. This workflow makes it easier to collaborate on changes as you can communicate via code instead of words. It's also very helpful in cases when the contributor is not familiar with git (obviously not the case here).

[mojavelinux]

The effort to support Ruby 1.8 until Asciidoctor 2.0.0.

It's not so much of an effort as just something that was never clearly defined. By moving to semantic versioning, we can do it (meaning drop it).

[junaruga]

It's a rather recent feature of GitHub and it's the default setting when contributing to this project.

That's great feature for the collaboration. I did not know that.

you can communicate via code instead of words

I agree. This is really helpful.

[mojavelinux]

Looks good! I'll get this merged tonight.

[ggrossetie]

We might need to patch Asciidoctor.js and more specifically: https://github.com/asciidoctor/asciidoctor.js/blob/master/lib/asciidoctor/js/opal_ext/thread_safe.rb

I need to check but since we are now using ::Concurrent::Hash, we may need to define a "fake" class.

[ggrossetie]

i think we really want to say that the assignment is not "thread safe" right ?

And we may need to use a ::Concurrent (Ruby) helper.

[mojavelinux]

right you are!

[mojavelinux]

@Mogztter yes, we'll need something mapped in Opal. Something like:

module Concurrent
  class Hash < ::Hash
  end
end

That will replace the existing patch, though it doesn't hurt to keep both for now.

[mojavelinux]

I made some updates. It should be sound now. I had forgotten about the case in factory.rb. (I am so looking forwarded to be rid of Ruby 1.8.7 from this code base).

[junaruga]

@Mogztter thanks for the checking! @mojavelinux thanks for the updates! I checked it.

[mojavelinux]

Thanks again! We can use this opportunity to test on AsciidoctorJ and Asciidoctor.js.

cc: @robertpanzer

[ggrossetie]

@mojavelinux We may have a missing test case because I didn't patch Asciidoctor.js and the build is still green...

[mojavelinux]

Keep in mind this is only hit when registering a custom converter class or using the template converter. And Asciidoctor.js may already hit the fallback. But that's just a guess.

[ggrossetie]

I removed Asciidoctor template.js from the test suite a while back I think that's why. I will add a test with a dummy custom converter.

EDIT: When I register a custom converter, the following warning is thrown asciidoctor: WARNING: gem 'concurrent-ruby' is not installed. This gem is recommended when registering custom converters. but otherwise it's working.