Bug #49943 for CGI: Needs discussion: wish: PSGI support in CGI.pm

Wed Sep 23 08:09:00 2009 MARKSTOS [...] cpan.org - Ticket created

CC:	miyagawa [...] bulknews.net
Subject:	Needs discussion: wish: PSGI support in CGI.pm

Miyagawa has proposed that CGI.pm include direct support for the PSGI specification. An initial implementation has already been completed and is available in the 'pgsi_support' branch of my repo: http://github.com/markstos/CGI.pm/commits/psgi_support I support the theory of the project, which ports WSGI to Perl, which seems to have proven valuable in the Python and Ruby communities. This post explains more: http://bulknews.typepad.com/blog/2009/09/psgi-perl-wsgi.html Before this work is merged into the core, a few things need to happen: 1. We need to agree on the exact API 2. It needs to be tested and documented 3. Lincoln Stein needs to approve it. Here are my concerns regarding the API as it is proposed: 1. I'm concerned that the CGI::PSGI module exists primarily to set a variable: $CGI::PSGI. Directly setting the global or using a pragma seems more consistent with existing features. 2. I'm concerned about adding *any* significant features to CGI.pm at this point. However, there is no easy way to add this functionality externally. Ideally, if this moves forward, it would be ideal to have a similar update released to CGI::Simple at the same time. Next, I'm interested in feedback from other CGI.pm users about the proposed change

Thu Sep 24 21:05:21 2009 yanick [...] babyl.dyndns.org - Correspondence added

Subject:	Re: [rt.cpan.org #49943] Needs discussion: wish: PSGI support in CGI.pm
Date:	Thu, 24 Sep 2009 21:07:45 -0400
To:	bug-CGI.pm [...] rt.cpan.org
From:	Yanick Champoux <yanick [...] babyl.dyndns.org>

MARKSTOS via RT wrote: Show quoted text

> Miyagawa has proposed that CGI.pm include direct support for the PSGI > specification. [..]

Show quoted text

> Next, I'm interested in feedback from other CGI.pm users about the > proposed change

I share Mark's caution about adding new functionality to CGI. It's a module with a long history used by a lot of people, and in consequence we want to be extra-careful with any changes. On the other hand, the modifications to support PSGI are fairly minimal, and shouldn't impact any of the current functionality of the module. In this specific case, I think my vote would go for including the PSGI support, but only after making sure the implemented API is sound, and after having tested it through and through.

Thu Sep 24 21:05:23 2009 The RT System itself - Status changed from 'new' to 'open'

Thu Sep 24 21:37:46 2009 miyagawa [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #49943] Needs discussion: wish: PSGI support in CGI.pm
Date:	Fri, 25 Sep 2009 10:37:29 +0900
To:	bug-CGI.pm [...] rt.cpan.org
From:	Tatsuhiko Miyagawa <miyagawa [...] gmail.com>

Show quoted text

> 1. I'm concerned that the CGI::PSGI module exists primarily to set a > variable: $CGI::PSGI. Directly setting the global or using a pragma > seems more consistent with existing features.

CGI::PSGI module is there to help ease the migration in web frameworks, so "use CGI::PSGI" will fail if the installed CGI.pm doesn't support PSGI. The problem here is that current (unpatched) CGI.pm DOESN'T fail when called "$CGI::PSGI = 1" nor "use CGI qw(:psgi)" and silently doesn't do anything useful re: PSGI variables. Making CGI::PSGI module just to set the flag then makes sure that your application bails out if you need PSGI support but when your CGI.pm doesn't support it. Also, your frameworks can say "requires 'CGI::PSGI'" in Makefile.PL, instead of specifying the CGI.pm version number. the API's awkwardness is something separate worth discussing. I wanted to say use CGI::PSGI; my $q = CGI::PSGI->new($env); my($status, $headers) = $q->headers($content_type); without touching CGI.pm at all, but it was really hard since CGI.pm directly manipulates %ENV and there are so many methods to clone. (That means it's not technically impossible, so we still keep that as an option if a direct PSGI support is unapproved). Thanks, -- Tatsuhiko Miyagawa

Thu Sep 24 22:10:07 2009 miyagawa [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #49943] Needs discussion: wish: PSGI support in CGI.pm
Date:	Fri, 25 Sep 2009 11:09:47 +0900
To:	bug-CGI.pm [...] rt.cpan.org
From:	Tatsuhiko Miyagawa <miyagawa [...] gmail.com>

On Fri, Sep 25, 2009 at 10:37 AM, miyagawa@gmail.com via RT <bug-CGI.pm@rt.cpan.org> wrote: Show quoted text

> The problem here is that current (unpatched) CGI.pm DOESN'T fail when > called "$CGI::PSGI = 1" nor "use CGI qw(:psgi)" and silently doesn't > do anything useful re: PSGI variables. Making CGI::PSGI module just to > set the flag then makes sure that your application bails out if you > need PSGI support but when your CGI.pm doesn't support it.

The other approach for this would be to let app developers to say: use CGI qw(:psgi); # this would set $CGI::PSGI to 1 die "Your CGI.pm doesn't support PSGI!" unless defined $CGI::PSGI; -- Tatsuhiko Miyagawa

Thu Sep 24 22:16:46 2009 miyagawa [...] gmail.com - Correspondence added

CC:	MARKSTOS [...] cpan.org
Subject:	Re: [rt.cpan.org #49943] Needs discussion: wish: PSGI support in CGI.pm
Date:	Fri, 25 Sep 2009 11:16:26 +0900
To:	bug-CGI.pm [...] rt.cpan.org
From:	Tatsuhiko Miyagawa <miyagawa [...] gmail.com>

On Fri, Sep 25, 2009 at 10:37 AM, miyagawa@gmail.com via RT <bug-CGI.pm@rt.cpan.org> wrote: Show quoted text

> use CGI::PSGI; > my $q = CGI::PSGI->new($env); > my($status, $headers) = $q->headers($content_type); > > without touching CGI.pm at all, but it was really hard since CGI.pm > directly manipulates %ENV and there are so many methods to clone. > (That means it's not technically impossible, so we still keep that as > an option if a direct PSGI support is unapproved).

The actual implementation for that would be: 1) in CGI::PSGI::new, save the current %ENV somewhere and populate %ENV with PSGI's $env 2) Override methods that reads from STDIN, like read_from_stdin and read_from_client to read from $env->{'psgi.input'} 3) Override header() method, or implement a different named method (like psgi_header) to return $status and $headers_ref 4) in DESTROY, restore the saved %ENV, and call CGI::initialize_globals() I *think* this can be done without touching CGI.pm at all, but a) a fair amount of code may need to be duplicated b) I'm not really sure that's "thread-safe" (or event-loop safe). If you run multiple PSGI apps in parallel, in our case with AnyEvent/Danga::Socket based event-loop server or with Coro server and if the app does streaming output (rather than direct response), global resetting and sharing of %ENV might cause issues. (Well, actually this might be problematic with the current patch as well though: local *ENV = $env gets reset when it goes out of scope -- same problem). -- Tatsuhiko Miyagawa

Fri Sep 25 00:24:41 2009 miyagawa [...] gmail.com - Correspondence added

CC:	MARKSTOS [...] cpan.org
Subject:	Re: [rt.cpan.org #49943] Needs discussion: wish: PSGI support in CGI.pm
Date:	Fri, 25 Sep 2009 13:24:23 +0900
To:	bug-CGI.pm [...] rt.cpan.org
From:	Tatsuhiko Miyagawa <miyagawa [...] gmail.com>

On Fri, Sep 25, 2009 at 11:16 AM, Tatsuhiko Miyagawa <miyagawa@gmail.com> wrote: Show quoted text

> The actual implementation for that would be: > > 1) in CGI::PSGI::new, save the current %ENV somewhere and populate > %ENV with PSGI's $env > 2) Override methods that reads from STDIN, like read_from_stdin and > read_from_client to read from $env->{'psgi.input'} > 3) Override header() method, or implement a different named method > (like psgi_header) to return $status and $headers_ref > 4) in DESTROY, restore the saved %ENV, and call CGI::initialize_globals()

Okay, I've done this: http://github.com/miyagawa/CGI-PSGI The implemenation is slightly modified, so PSGI $env is stored in the object and set to local *ENV whenever a method that touches %ENV is called. This might have a slight performance impact but I don't think it's really a problem. I also left $q->header as it was, and added a new $q->psgi_header method instead. This is not a big deal since app developers need to change wherever $q->header was called to do print(). The benefit of new implementation is, as stated in README and POD, that it doesn't require CGI.pm patched to support PSGI natively. Mark, let's remove CGI::PSGI from CGI.pm main repo as you suggested and use $CGI::PSGI variable's defined-ness to see if CGI.pm supports PSGI or not. I think I can ship this CGI::PSGI to CPAN, and then CGI.pm can individually support PSGI with $CGI::PSGI or use CGI qw(:psgi); and both can live on CPAN: application users can chose whichever they want to. a) If they don't want to upgrade CGI.pm and are fine with changing their code, use CGI::PSGI b) If they don't want to change their CGI.pm based code, then upgrade CGI.pm that fully supports PSGI By "fully supports PSGI" the current CGI.pm (my patch) is not complete since it only does the read side. The app should still update their code to do return [ $q->header, [ $body ] ] instead of print $q->header, $body; To enable that you need to tie STDOUT, which actually CGI::Emulate::PSGI generally solves, in which case CGI.pm doesn't really need to be patched. Hmm ... ;) -- Tatsuhiko Miyagawa

Tue Jan 22 21:16:02 2013 MARKSTOS [...] cpan.org - Correspondence added

RT-Send-CC:

miyagawa [...] bulknews.net

I'm not sure there's anything else to do here. Resolving.

Tue Jan 22 21:16:04 2013 MARKSTOS [...] cpan.org - Status changed from 'open' to 'resolved'

Fri May 23 14:29:34 2014 The RT System itself - Queue changed from CGI.pm to CGI

Bug #49943 for CGI: Needs discussion: wish: PSGI support in CGI.pm

Preferred bug tracker