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

Replacing JSON::XS dependency with a thread safe Module #34

Open
abhipranay opened this issue Oct 23, 2017 · 5 comments
Open

Replacing JSON::XS dependency with a thread safe Module #34

abhipranay opened this issue Oct 23, 2017 · 5 comments

Comments

@abhipranay
Copy link

Calling Solr functions inside a Perl thread is resulting in exception because of JSON:XS module is not able to handle the responses correctly.

File: WebService/Solr/Response.pm
sub _build_content

Can we use a thread safe module for json parsing purposes ?
https://metacpan.org/pod/Cpanel::JSON::XS

@petdance
Copy link
Owner

I'm not immediately opposed to the idea of having WebService::Solr having some way to use one of two modules, but it's a pretty big change that will introduce a lot of maintenance headaches.

I'd like to know more backstory of why it was necessary to fork Cpanel::JSON::XS from JSON::XS. Is there no way to get the fixes into JSON::XS? The docs for Cpanel::JSON::XS sat there is no public bug tracker for JSON::XS, but this looks like bug tracking: https://rt.cpan.org/Public/Dist/Display.html?Name=JSON-XS

The Cpanel::JSON::XS docs also say

As this is the n-th-something JSON module on CPAN, what was the reason to write yet another JSON module? While it seems there are many JSON modules, none of them correctly handle all corner cases, and in most cases their maintainers are unresponsive, gone missing, or not listening to bug reports for other reasons.

which brings up the question: Is Cpanel::JSON::XS necessarily the one to provide as an option?

@jberger
Copy link

jberger commented Oct 23, 2017

The fork was originally necessary because the author felt that the hash ordering change in Perl 5.18 was incorrect and chose not to fix JSON::XS it for the entire 5.17 development cycle.
so anyone working on the development of 5.18 was broken. He did release a fix the day of the 5.18 release but by then it was forked.

Since the fork they have diverged slightly. C::J::XS now uses a better heuristic to detect number vs string (the same one that Mojo::JSON now uses too). It also uses the JSON::PP::Booleans for standardizing booleans for interoperability (as Mojo::JSON does) and JSON::XS does not. I don't know about thread-safety differences because I don't use perl threads, but I wouldn't be surprised if there were differences there, the author of JSON::XS isn't a believer in Perl threading (to other disastrous consequences (see Coro and its surrounding flap)).

For the purposes of this module, you might want to use JSON::MaybeXS, which chooses the best JSON module available on the user's computer and IIRC you can blacklist implementations.

@petdance
Copy link
Owner

https://perlmaven.com/json

@abhipranay
Copy link
Author

Using JSON::MaybeXS will again make things little inflexible for purpose of this module as definition of 'best' json module will be JSON::MaybeXS decison.
Better to keep this decision making under WebService::Solr codebase.
If it seems like an overkill as we are using decode_json seldomly we can directly use Cpanel::JSON::XS

@jjatria
Copy link

jjatria commented Sep 27, 2019

FWIW, I would also like to see this switch to JSON::MaybeXS. The previous comment said this would make it more inflexible, but I don't think this is the case.

The definition of the "best" module comes from what is installed in the user's system. If C:J:XS is installed, it will use that one. If not, it will fall back to J:XS, and if not, to J:PP.

Switching to JSON::MaybeXS should also help alleviate some of the maintenance headaches of a switch in the underlying JSON library: only users that have both J:XS and C:J:XS will be affected (rather than all users).

As it stands, using J:XS as a dependency means that this cannot be installed in systems where XS is not available, and that if you already prefer using C:J:XS you'll have two JSON distributions in your system. Switching to JSON::MaybeXS would fix both of these issues (as well as the one that opened this thread).

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

No branches or pull requests

4 participants