search

DCIT / perl-CryptX

https://metacpan.org/pod/CryptX
Other
34 stars 23 forks source link

Is it possible to add a $SIG{'__DIE__'} handler to the CryptX.pm file? #72

Closed rodolfojcj closed 3 years ago

rodolfojcj commented 3 years ago

Hello.

What do you think about improving the lib/CryptX.pm file to add a $SIG{'__DIE__'} handler, so that its BEGIN block looks like this:

BEGIN {
  local $SIG{'__DIE__'} = sub {
    # either an empty subroutine
    # or maybe some warning message here
  };

  if (eval { require Cpanel::JSON::XS }) {
    Cpanel::JSON::XS->import(qw(encode_json decode_json));
    $has_json = 1;
  }
  elsif (eval { require JSON::XS }) {
    JSON::XS->import(qw(encode_json decode_json));
    $has_json = 2;
  }
  elsif (eval { require JSON::PP }) {
    JSON::PP->import(qw(encode_json decode_json));
    $has_json = 3;
  }
  else {
    $has_json = 0;
  }
}

The reason is that without such $SIG{'__DIE__'} handler, the execution of any of the three eval { require ... } statements generates __DIE__ signals when either Cpanel::JSON::XS or JSON::XS or JSON::PP are not installed in the system.

A symptom of that situation is that messages like this are fired up to the calling code:

Can't locate Cpanel/JSON/XS.pm in @INC (you may need to install the Cpanel::JSON::XS module)
(@INC contains:
/opt/myprogram/current/perllib
/etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.30.0
/usr/local/share/perl/5.30.0
/usr/lib/x86_64-linux-gnu/perl5/5.30
/usr/share/perl5
/usr/lib/x86_64-linux-gnu/perl/5.30
/usr/share/perl/5.30 /usr/local/lib/site_perl)
at /usr/local/lib/x86_64-linux-gnu/perl/5.30.0/CryptX.pm line 14.

That message comes from a local installation of mine, which does not have Cpanel::JSON::XS installed.

I think that change is justified by the following explanation given at the Perl documentation for the eval-BLOCK form:

Using the eval {} form as an exception trap in libraries does have some issues. Due to the current arguably broken state of __DIE__ hooks, you may wish not to trigger any __DIE__ hooks that user code may have installed. You can use the local $SIG{__DIE__} construct for this purpose, as this example shows:

# a private exception trap for divide-by-zero
eval { local $SIG{'__DIE__'}; $answer = $a / $b; };
warn $@ if $@;

Also, the Perl documentation for the %SIG variable mentions this:

The $SIG{__DIE__} hook is called even inside an eval(). It was never intended to happen this way, but an implementation glitch made this possible. This used to be deprecated, as it allowed strange action at a distance like rewriting a pending exception in $@. Plans to rectify this have been scrapped, as users found that rewriting a pending exception is actually a useful feature, and not a bug.

In my particular case, the proposed change will help with these:

If I can help with something, please let me know.

Any other alternative implementation, or any other suggestion you may have to handle such __DIE__ signals, will also be welcome.

Thanks in advance.

karel-m commented 3 years ago

Would it help if I switch to pure JSON as I did in Crypt::JWT? I do not like the idea of __DIE__ handler.

rodolfojcj commented 3 years ago

Does it mean that Cpanel::JSON::XS would not be needed? If yes, I assume that will work for my use case, given that the system already has these modules installed:

Thank you.

rodolfojcj commented 3 years ago

Hello.

I would like to know if you plan to implement the change previously mentioned at https://github.com/DCIT/perl-CryptX/issues/72#issuecomment-848181572?

If yes, do you estimate a date for a release containing it?

Thanks again.

karel-m commented 3 years ago

Yes, I plan to switch to pure JSON.

Grinnz commented 3 years ago

FYI, this is a regression as JSON.pm is a poor wrapper module which does not use Cpanel::JSON::XS when available - JSON::XS contains a number of bugs so it's best to allow the option of using the fork. If you want to depend on a single wrapper module, I suggest https://metacpan.org/pod/JSON::MaybeXS