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

Test failure on 5.17.6 #870

Closed
bigpresh opened this issue Dec 5, 2012 · 5 comments
Closed

Test failure on 5.17.6 #870

bigpresh opened this issue Dec 5, 2012 · 5 comments

Comments

@bigpresh
Copy link
Member

bigpresh commented Dec 5, 2012

In checking whether our tests will get bitten by the upcoming hash key ordering changes, I just tried building Dancer using 5.17.6, and the tests failed with:

Can't locate object method "read" via package "GLOB(0x3168728)" (perhaps you forgot to load "GLOB(0x3168728)"?
) at /home/davidp/.cpanm/work/1354733219.26160/Dancer-1.3110/blib/lib/Dancer/Request.pm line 524

The line in question is:

$rc = $self->input_handle->read($buffer, $readlen);

$self->input_handle() looks like:

sub input_handle   { $_[0]->env->{'psgi.input'} || $_[0]->env->{'PSGI.INPUT'} }

The PSGI docs say that the psgi.input env var has the following to say:

The input stream in psgi.input is an IO::Handle-like object which streams the raw HTTP POST or PUT data. If it is a file handle then it MUST be opened in binary mode. The input stream MUST respond to read and MAY implement seek.

It's entirely possible that the problem lies with PSGI, not Dancer, but it needs investigation.

@bigpresh
Copy link
Member Author

bigpresh commented Dec 5, 2012

Having looked at the code in Plack's HTTP::Message::PSGI which sets psgi.input, it seems it will either be a HTTP::Message::PSGI::ChunkedInput instance, or a normal glob filehandle opened with open $input, "<", \$content;

It looks like a normal filehandle opened that way won't be blessed by default:

#!/usr/bin/perl

use strict;

my $foo = "Foo bar";
open my $fh, "<", \$foo
    or die "Failed to open - $!";;

say "fh is a " . ref $fh;

if ($fh->can('read')) {
    say "Yep, can read()";
} else {
    say "No read()";
}

gives me the following:

fh is a GLOB
No read()

So, it would seem that that code in Plack, returning a straightforward filehandle, contravenes the bit of the Plack docs I quoted earlier. I'll double check this is actually the case, then report it.

@kentfredric
Copy link
Contributor

It would appear the problem is that for whatever reason, env->{'psgi.input'} is literally a stringified glob, so something higher up is failing to inser the right value into ENV().

Just where exactly its failing is hard to work out.

ie: it doesn't matter if its blessed or not, if its a filehandle at all, then it would work, ( Because of how IO::Handle autoloading works, all filehandles behave like blessed objects ).

Just to make sure, I placed the following code in _read

    my $handle;

    $handle = $self->input_handle;

    if ( Scalar::Util::blessed( $handle ) ){
        $rc = $handle->read( $buffer, $readlen );
    } else {
        $rc = read( $handle, $buffer, $readlen );
    }

The problem is of course, $handle in this case is a string containing GLOB(0xDEADBEEF), and the referrent glob is long gone, so this doesn't solve the problem.a

The problem is to work out /why/ the glob got wrongly stringified in $ENV.

@kentfredric
Copy link
Contributor

If you want to catch the error a little earlier:

Dancer/Request.pm

sub new {
    my ($self, @args) = @_;
    if (@args == 1) {
        @args = ('env' => $args[0]);
        Dancer::Deprecation->deprecated(
                      fatal   => 1,
                      feature => 'Calling Dancer::Request->new($env)',
                      version => 1.3059,
                      reason  => 'Please use Dancer::Request->new( env => $env ) instead',
         );
    }
    if( @args == 2 and $args[0] eq 'env' ){
        for my $check ( qw( psgi.input PSGI.INPUT ) ) {
            if( exists $args[1]->{$check} and not ref $args[1]->{$check} ) { 
                require Carp;
                Carp::confess( "Non-Reference $check == " . $args[1]->{$check} );
            }
        }
    }
    $self->SUPER::new(@args);
}

This helps a bit,

@kentfredric
Copy link
Contributor

Seems the problem is using the magical variable %ENV to store data.

%ENV has a magical property ( demonstrated by this code ) that even values are stringified.

use strict;
use warnings;


my %hash;

{ 
    my $content = "hello";
    open my $fh, '<', \$content;
    $ENV{'foo'} = $fh;
    $hash{'foo'} = $fh;
}


print "ENV{'foo'} is " . ( ref $ENV{'foo'} ? '' :  'not ') . "a ref\n";
print "hash{'foo'} is " . ( ref $hash{'foo'} ? '' : 'not ') . "a ref\n";
ENV{'foo'} is not a ref
hash{'foo'} is a ref

So the only way to solve this problem is stop using the magical variable %ENV to pass references around.

kentfredric added a commit to kentfredric/Dancer that referenced this issue Dec 15, 2012
Perl 5.17.3+ changes behaviour of `%ENV` so values are also stringified.

This change aims to solve that ( gh PerlDancer#870 ) by replacing %ENV based code with
a passed around local hashref, that is passed as the last parameter to `new_for_request()`
instead of relying on the data being passed around via `%ENV`.
@kentfredric kentfredric mentioned this issue Dec 15, 2012
@yanick
Copy link
Contributor

yanick commented Mar 21, 2013

Fixed by #874, closing.

@yanick yanick closed this as completed Mar 21, 2013
Perlover pushed a commit to Perlover/Dancer that referenced this issue Sep 13, 2017
Perl 5.17.3+ changes behaviour of `%ENV` so values are also stringified.

This change aims to solve that ( gh PerlDancer#870 ) by replacing %ENV based code with
a passed around local hashref, that is passed as the last parameter to `new_for_request()`
instead of relying on the data being passed around via `%ENV`.

Conflicts:

	lib/Dancer/Request.pm
	lib/Dancer/Test.pm
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

3 participants