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

MultiJSON assumes that the JSON ext is loaded if defined?(::JSON::JSON_LOADED) #196

Closed
eregon opened this issue Jun 29, 2020 · 4 comments · Fixed by #197
Closed

MultiJSON assumes that the JSON ext is loaded if defined?(::JSON::JSON_LOADED) #196

eregon opened this issue Jun 29, 2020 · 4 comments · Fixed by #197

Comments

@eregon
Copy link
Contributor

eregon commented Jun 29, 2020

See

return :json_gem if defined?(::JSON::JSON_LOADED)

But that is unfortunately incorrect, because JSON pure also defines that constant:
https://github.com/flori/json/blob/b20cdca8eb03297132bcf3b786eba602fa04ca37/lib/json/pure.rb#L14

I think using defined?(::JSON::Ext) would work better instead.

This causes an issue on TruffleRuby where JSON pure is used by default (and require 'json/ext' raises unless the json gem is installed additionally).
If json is required before multi_json, then multi_json raises an error: oracle/truffleruby#2032 (comment)

On MRI it's quite confusing as it will detect JSON pure as :json_gem (same issue), but still work because it will then require 'json/ext' which will work (but have 2 versions of JSON loaded, so it seems suboptimal).

@eregon
Copy link
Contributor Author

eregon commented Jun 29, 2020

I'll try to make a PR for this.

@eregon
Copy link
Contributor Author

eregon commented Jun 30, 2020

An additional confusing thing is that json_pure seems to ship a json_pure-2.3.0/lib/json/ext.rb file which defines JSON::Ext.
But I guess if something require 'json/ext' they really meant to use the native JSON, even if json_pure was earlier in the load path and we end up with something like:

$ ruby -d -e 'gem "json_pure"; require "json/pure"; require "json/ext"; puts $"; p JSON::Ext'
Using Pure library for JSON.
Using Ext extension for JSON.
...
/home/eregon/.rubies/ruby-2.6.6/lib/ruby/gems/2.6.0/gems/json_pure-2.3.0/lib/json/pure.rb
/home/eregon/.rubies/ruby-2.6.6/lib/ruby/gems/2.6.0/gems/json-2.3.0/lib/json/ext/parser.so
/home/eregon/.rubies/ruby-2.6.6/lib/ruby/gems/2.6.0/gems/json-2.3.0/lib/json/ext/generator.so
/home/eregon/.rubies/ruby-2.6.6/lib/ruby/gems/2.6.0/gems/json_pure-2.3.0/lib/json/ext.rb

@eregon
Copy link
Contributor Author

eregon commented Jun 30, 2020

So I guess we should check for JSON::Ext::Parser (or JSON::Ext::Generator).
That's not defined, even after require "json/ext" if there is no C extension.

eregon added a commit to eregon/multi_json that referenced this issue Jun 30, 2020
* JSON::JSON_LOADED is also set by json/pure and so it is not enough to
  detect if the JSON native extension is loaded.
* JSON::Ext is not enough either, because json_pure includes json/ext.rb.
* JSON::Ext::Parser is only defined if the native extension is loaded so
  that is reliable.
* Fixes intridea#196
* See that issue for details.
eregon added a commit to eregon/multi_json that referenced this issue Jun 30, 2020
* JSON::JSON_LOADED is also set by json/pure and so it is not enough to
  detect if the JSON native extension is loaded.
* JSON::Ext is not enough either, because json_pure includes json/ext.rb.
* JSON::Ext::Parser is only defined if the native extension is loaded so
  that is reliable.
* Fixes intridea#196
* See that issue for details.
@eregon
Copy link
Contributor Author

eregon commented Jun 30, 2020

PR to fix this: #197

@rwz rwz closed this as completed in #197 Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant