log4perl-devel

[log4perl-devel] Log::Log4Perl::Util::module_available broken...

[Merijn B. <me...@il...>]

Hello,

There is an existing RT bug for the issue of module_available being
broken already:

http://rt.cpan.org/Public/Bug/Display.html?id=24884

which is 10 months older then the release done yesterday. Since there
have been tickets closed since then I am assuming you guys are aware of
this issue already. 

I just wanted to reiterate that this code really is broken, both in
spirit and in implementation. Please reconsider having this broken code
in such a useful package. The patch in RT is sufficient to restore
functionality, but I would personally take out all module_available
calls everywhere from Log4perl and replace it with a simple eval {require }. 

To illustrate implementation brokenness, see line 33 of Util.pm:

	ref $dir !~ /^(GLOB|SCALAR|HASH|REF|LVALUE)$/) {

which tries to determine the reference status on the result of a regular
expression. I assume the author wanted to write this :

	ref($dir) !~ /^(GLOB|SCALAR|HASH|REF|LVALUE)$/) {

but forgot about operator precedence. So this test will *never* be true. 
Which is good, as the code on line 34 inside the if branch is bad as well. 

	return 1 if $dir->INC();

as the INC method is supposed to return the open file handle of the file
requested. Since you do not request a file, the method is under no
obligation to actually return a true value.

But it would be pointless to try fixing this implementation since
whatever the result of the module_available function is, it just leads
to an eval { require } afterwards anyway, so you are doubling the
effort. It would be very nice if you would consider to modify Log4perl
to not use module_available. Or accept the RT patch. The current code
does not work with @INC objects. 

Regards,
-- 
Merijn Broeren | We take risks, we know we take them. Therefore, when things
               | come out against us, we have no cause for complaint.
               | - Scott, last journal entry, march 1912

Re: [log4perl-devel] Log::Log4Perl::Util::module_available broken...

[Mike S. <m...@pe...>]

On Thu, 22 Nov 2007, Merijn Broeren wrote:

> There is an existing RT bug for the issue of module_available being
> broken already:
>
> http://rt.cpan.org/Public/Bug/Display.html?id=24884
>
> which is 10 months older then the release done yesterday. Since there
> have been tickets closed since then I am assuming you guys are aware of
> this issue already.

Yeah, that's an old issue that dates back to Log4perl 0.42: There we
figured out that if someone wrote

    $SIG{__DIE__} = sub { print "die handler!\n"; };

and used Log4perl and some module like Time::HiRes is missing, which
is acceptable, the handler gets triggered because eval { require }
causes an exception. Confusion ensued.

Now as it has been pointed out, it's actually the user's fault. A
correct die-Handler checks if it was called because of an eval and
ignores the exception.

Now, I've always thought that this is a bug in perl, nobody writes die
die handlers 'correctly'. To comply with users who get confused with this
confusing behaviour, I put this module_available() hack in place, which,
admittedly, sucks.

What to do? Going back to eval { require } would break every user with
sloppy die handlers.

We could add a load time option to establish the correct behavior:

    use Log::Log4perl qw(:module_check_via_eval);

Other ideas?

-- Mike

Mike Schilli
m...@pe...

>
> I just wanted to reiterate that this code really is broken, both in
> spirit and in implementation. Please reconsider having this broken code
> in such a useful package. The patch in RT is sufficient to restore
> functionality, but I would personally take out all module_available
> calls everywhere from Log4perl and replace it with a simple eval {require }.
>
> To illustrate implementation brokenness, see line 33 of Util.pm:
>
>       ref $dir !~ /^(GLOB|SCALAR|HASH|REF|LVALUE)$/) {
>
> which tries to determine the reference status on the result of a regular
> expression. I assume the author wanted to write this :
>
>       ref($dir) !~ /^(GLOB|SCALAR|HASH|REF|LVALUE)$/) {
>
> but forgot about operator precedence. So this test will *never* be true.
> Which is good, as the code on line 34 inside the if branch is bad as well.
>
>       return 1 if $dir->INC();
>
> as the INC method is supposed to return the open file handle of the file
> requested. Since you do not request a file, the method is under no
> obligation to actually return a true value.
>
> But it would be pointless to try fixing this implementation since
> whatever the result of the module_available function is, it just leads
> to an eval { require } afterwards anyway, so you are doubling the
> effort. It would be very nice if you would consider to modify Log4perl
> to not use module_available. Or accept the RT patch. The current code
> does not work with @INC objects.
>
> Regards,
> --
> Merijn Broeren | We take risks, we know we take them. Therefore, when things
>                | come out against us, we have no cause for complaint.
>                | - Scott, last journal entry, march 1912
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Microsoft
> Defy all challenges. Microsoft(R) Visual Studio 2005.
> http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
> _______________________________________________
> log4perl-devel mailing list
> log...@li...
> https://lists.sourceforge.net/lists/listinfo/log4perl-devel
>

Re: [log4perl-devel] Log::Log4Perl::Util::module_available broken...

[Merijn B. <me...@il...>]

Quoting Mike Schilli (m...@pe...):
> 
> Yeah, that's an old issue that dates back to Log4perl 0.42: There we
> figured out that if someone wrote
> 
>     $SIG{__DIE__} = sub { print "die handler!\n"; };
> 
> and used Log4perl and some module like Time::HiRes is missing, which
> is acceptable, the handler gets triggered because eval { require }
> causes an exception. Confusion ensued.
> 
Ok, so you have a good reason for attempting this, which is great to
know. I was really wondering why you would do this. 

> Now, I've always thought that this is a bug in perl, nobody writes die
> die handlers 'correctly'. To comply with users who get confused with this
> confusing behaviour, I put this module_available() hack in place, which,
> admittedly, sucks.
> 
;-)

> What to do? Going back to eval { require } would break every user with
> sloppy die handlers.
> 
No, if you read the patch on rt closely, you will see it puts a 'local'
die handler in place, just for the scope of the eval. This will
temporarily override all other die handlers, safely do an eval and then
return the result and unmask the users die handlers afterwards. This is
entirely safe to do and the eval will not trigger the users die handlers
anymore. You can just apply the patch as is and you will not break users
code.

Regards,
-- 
Merijn Broeren | We take risks, we know we take them. Therefore, when things
               | come out against us, we have no cause for complaint.
               | - Scott, last journal entry, march 1912