Mailing List Archive

Minor Catalyst::Script::Server bug - with patch!
I found a minor bug in Catalyst today. It showed up in our development environment, and made me crazy trying to track it down.

If you have a MyApp::Script::Server class, when you run myapp_server.pl, it will sometimes miss checking the values for MYAPP_PORT and MYAPP_RELOAD from the environment.

It's the "sometimes" that made this hard to sort out.

As far as I can tell, the values from the constructor to the Server object are being set up in a different order. Sometimes application_name is set up before port or reload, in which case the right app-specific environment variables are checked. Sometimes port or reload (or both) are set up before application_name is initialized, and in those cases 'MyApp' isn't sent to env_value, so the MYAPP variant of the environment block isn't checked.

I never did figure out what caused the order to be different. I can guess wildly, but don't really know for sure.

The solution - already used in one of the accessors in Catalyst::Script::Server - is to make port and reload lazy, so application_name will always be set before they're called. Any of the accessors that use env_value in their default should be lazy so you can depend on application_name. It's a trivial change.

It did break one of the tests, though, as the test uses direct hash access to test retrieved values. For, as far as I can tell, no good reason. Changing the test to a method call fixes it.

I couldn't figure out how to write a test to trigger this every time, so I have no idea how I'd write a test to prove it's really fixed.

Here, as requested on the web site, are diffs. If there's a better way to get them to you, I do have this checked out in git and could push them someplace or something.

Let me know and I'll be happy to do so, so no one else has to spend half a day scratching their head over this one.

diff -u -b lib/Catalyst/Script/Server-orig.pm lib/Catalyst/Script/Server.pm
--- lib/Catalyst/Script/Server-orig.pm 2014-05-26 20:03:00.000000000 -0700
+++ lib/Catalyst/Script/Server.pm 2014-05-26 20:02:23.000000000 -0700
@@ -37,6 +37,7 @@
cmd_aliases => 'p',
isa => 'Int',
is => 'ro',
+ lazy => 1,
default => sub {
Catalyst::Utils::env_value(shift->application_name, 'port') || 3000
},
@@ -107,6 +108,7 @@
cmd_aliases => 'r',
isa => 'Bool',
is => 'ro',
+ lazy => 1,
default => sub {
Catalyst::Utils::env_value(shift->application_name, 'reload') || 0;
},

diff -u -b t/aggregate/unit_core_script_server-orig.t t/aggregate/unit_core_script_server.t
--- t/aggregate/unit_core_script_server-orig.t 2014-05-26 20:13:25.000000000 -0700
+++ t/aggregate/unit_core_script_server.t 2014-05-26 20:13:34.000000000 -0700
@@ -151,7 +151,7 @@

## Check a few args
is_deeply $app->{ARGV}, $argstring;
- is $app->{port}, '3000';
+ is $app->port, '3000';
is($app->{background}, 1);
}
Re: Minor Catalyst::Script::Server bug - with patch! [ In reply to ]
Hey,

Looks pretty good, any chance we could get a patch via github : perl-catalyst/catalyst-runtime?

perl-catalyst/catalyst-runtime
catalyst-runtime - The Elegant MVC Web Application Framework
View on github.com Preview by Yahoo

It just makes it easier for me to track changes and accountability.  Thanks!  --John


On Monday, May 26, 2014 11:19 PM, Louis Erickson <lerickson@rdwarf.net> wrote:



I found a minor bug in Catalyst today.  It showed up in our development environment, and made me crazy trying to track it down.

If you have a MyApp::Script::Server class, when you run myapp_server.pl, it will sometimes miss checking the values for MYAPP_PORT and MYAPP_RELOAD from the environment.

It's the "sometimes" that made this hard to sort out.

As far as I can tell, the values from the constructor to the Server object are being set up in a different order.  Sometimes application_name is set up before port or reload, in which case the right app-specific environment variables are checked.  Sometimes port or reload (or both) are set up before application_name is initialized, and in those cases 'MyApp' isn't sent to env_value, so the MYAPP variant of the environment block isn't checked.

I never did figure out what caused the order to be different.  I can guess wildly, but don't really know for sure.

The solution - already used in one of the accessors in Catalyst::Script::Server - is to make port and reload lazy, so application_name will always be set before they're called.  Any of the accessors that use env_value in their default should be lazy so you can depend on application_name.  It's a trivial change.

It did break one of the tests, though, as the test uses direct hash access to test retrieved values.  For, as far as I can tell, no good reason.  Changing the test to a method call fixes it.

I couldn't figure out how to write a test to trigger this every time, so I have no idea how I'd write a test to prove it's really fixed.

Here, as requested on the web site, are diffs.  If there's a better way to get them to you, I do have this checked out in git and could push them someplace or something.

Let me know and I'll be happy to do so, so no one else has to spend half a day scratching their head over this one.

diff -u -b lib/Catalyst/Script/Server-orig.pm lib/Catalyst/Script/Server.pm 
--- lib/Catalyst/Script/Server-orig.pm2014-05-26 20:03:00.000000000 -0700
+++ lib/Catalyst/Script/Server.pm2014-05-26 20:02:23.000000000 -0700
@@ -37,6 +37,7 @@
     cmd_aliases   => 'p',
     isa           => 'Int',
     is            => 'ro',
+    lazy          => 1,
     default       => sub {
         Catalyst::Utils::env_value(shift->application_name, 'port') || 3000
     },
@@ -107,6 +108,7 @@
     cmd_aliases   => 'r',
     isa           => 'Bool',
     is            => 'ro',
+    lazy          => 1,
     default       => sub {
         Catalyst::Utils::env_value(shift->application_name, 'reload') || 0;
     },

diff -u -b t/aggregate/unit_core_script_server-orig.t t/aggregate/unit_core_script_server.t 
--- t/aggregate/unit_core_script_server-orig.t2014-05-26 20:13:25.000000000 -0700
+++ t/aggregate/unit_core_script_server.t2014-05-26 20:13:34.000000000 -0700
@@ -151,7 +151,7 @@
 

     ## Check a few args
     is_deeply $app->{ARGV}, $argstring;
-    is $app->{port}, '3000';
+    is $app->port, '3000';
     is($app->{background}, 1);
 }
 





_______________________________________________
Catalyst-dev mailing list
Catalyst-dev@lists.scsys.co.uk
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev
Re: Minor Catalyst::Script::Server bug - with patch! [ In reply to ]
On May 29, 2014, at 7:58 AM, John Napiorkowski <jjn1056@yahoo.com> wrote:

> Hey,
>
> Looks pretty good, any chance we could get a patch via github : perl-catalyst/catalyst-runtime?

Happy to do so, if I can figure out how.

I didn't know Catalyst was using github. The Wiki points to a git repository at shadowcat.

I've used github less than the rest of git; what's the preferred process to get you the patch? Their documentation suggests I fork, submit to my fork, then issue a "pull request"... is that what you need?

If so, I'll try that tonight.
Re: Minor Catalyst::Script::Server bug - with patch! [ In reply to ]
Apparently "tonight" was optimistic, as that was nearly a month ago. I have finally submitted a pull request to you. It should be waiting for you in github.

There was one error before and after my changes - t/author/spelling.t failed. I'll probably submit a patch for that, too because having make test fail annoyed me.

In my environment, it's quite useful to have an additional option to the server as well, '-h' to select which hostname to use. Testing and winding up on the wrong interface is not my favorite. I've been adding that to each app, but it appears to me to be a pretty generic option. If I send along a pull request to add it, will you consider it? (I'll have to find the documentation, and update that, too.)

On Jun 2, 2014, at 2:25 PM, Louis Erickson <lerickson@rdwarf.net> wrote:

>
> On May 29, 2014, at 7:58 AM, John Napiorkowski <jjn1056@yahoo.com> wrote:
>
>> Hey,
>>
>> Looks pretty good, any chance we could get a patch via github : perl-catalyst/catalyst-runtime?
>
> Happy to do so, if I can figure out how.
>
> I didn't know Catalyst was using github. The Wiki points to a git repository at shadowcat.
>
> I've used github less than the rest of git; what's the preferred process to get you the patch? Their documentation suggests I fork, submit to my fork, then issue a "pull request"... is that what you need?
>
> If so, I'll try that tonight.
>
> _______________________________________________
> Catalyst-dev mailing list
> Catalyst-dev@lists.scsys.co.uk
> http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev
Re: Minor Catalyst::Script::Server bug - with patch! [ In reply to ]
I think I've done this right. Git is making me slightly crazy; I've been using Perforce for ten years, and there are things that feel similar but aren't.

I've submitted a pull request for the stopwords to get t/author/spelling.t fixed. Trivial, but it makes the tests pass.

I misremembered what it is I've been having to override Script/Server.pm to provide; it isn't an option for the hostname; there already is a -h option for that. It's an environment variable to set the hostname, like you can set the port number. I've got a change for that, too. Shall I send the pull request for that?

On Jun 25, 2014, at 8:35 PM, Louis Erickson <lerickson@rdwarf.net> wrote:

> Apparently "tonight" was optimistic, as that was nearly a month ago. I have finally submitted a pull request to you. It should be waiting for you in github.
>
> There was one error before and after my changes - t/author/spelling.t failed. I'll probably submit a patch for that, too because having make test fail annoyed me.
>
> In my environment, it's quite useful to have an additional option to the server as well, '-h' to select which hostname to use. Testing and winding up on the wrong interface is not my favorite. I've been adding that to each app, but it appears to me to be a pretty generic option. If I send along a pull request to add it, will you consider it? (I'll have to find the documentation, and update that, too.)
>
> On Jun 2, 2014, at 2:25 PM, Louis Erickson <lerickson@rdwarf.net> wrote:
>
>>
>> On May 29, 2014, at 7:58 AM, John Napiorkowski <jjn1056@yahoo.com> wrote:
>>
>>> Hey,
>>>
>>> Looks pretty good, any chance we could get a patch via github : perl-catalyst/catalyst-runtime?
>>
>> Happy to do so, if I can figure out how.
>>
>> I didn't know Catalyst was using github. The Wiki points to a git repository at shadowcat.
>>
>> I've used github less than the rest of git; what's the preferred process to get you the patch? Their documentation suggests I fork, submit to my fork, then issue a "pull request"... is that what you need?
>>
>> If so, I'll try that tonight.
>>
>> _______________________________________________
>> Catalyst-dev mailing list
>> Catalyst-dev@lists.scsys.co.uk
>> http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev
>
> _______________________________________________
> Catalyst-dev mailing list
> Catalyst-dev@lists.scsys.co.uk
> http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev