Mailing List Archive: Catalyst: Dev

Fix for system() returning -1 on dev server

Index | Next | Previous | View Threaded

jon+catalyst at youramigo

Oct 25, 2007, 8:33 PM

Post #1 of 6 (1817 views)
Permalink

Fix for system() returning -1 on dev server

That system() returns -1 while running on the Catalyst::Engine::HTTP
server seems to be a well known fact and was discussed on the list a
year ago (didn't stop the bug biting me again but).

I have traced the cause to HTTP.pm, line 191 or thereabouts:

local $SIG{CHLD} = 'IGNORE';

Getting rid of this line, or setting $SIG{CHLD} to a proper child reaper
handler, fixes the problem with system().

There's a warning in perlvar that $? may be incorrect if SIG{CHLD} is
defined; presumably it's either that or that defining the IGNORE handler
interferes with the internals of the system() call for retrieving the
exit status.

Patch against svn that sets an appropriate reaper is attached.

--

Jon

Attachments:

catalyst-http-patch.txt (0.78 KB)

andy at hybridized

Oct 25, 2007, 9:02 PM

Post #2 of 6 (1696 views)
Permalink

Re: Fix for system() returning -1 on dev server [In reply to]

On Oct 25, 2007, at 11:33 PM, Jon Schutz wrote:

> That system() returns -1 while running on the Catalyst::Engine::HTTP
> server seems to be a well known fact and was discussed on the list a
> year ago (didn't stop the bug biting me again but).
>
> I have traced the cause to HTTP.pm, line 191 or thereabouts:
>
> local $SIG{CHLD} = 'IGNORE';
>
> Getting rid of this line, or setting $SIG{CHLD} to a proper child
> reaper
> handler, fixes the problem with system().
>
> There's a warning in perlvar that $? may be incorrect if SIG{CHLD} is
> defined; presumably it's either that or that defining the IGNORE
> handler
> interferes with the internals of the system() call for retrieving the
> exit status.
>
> Patch against svn that sets an appropriate reaper is attached.

Thanks, applied!

_______________________________________________
Catalyst-dev mailing list
Catalyst-dev [at] lists
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

dbix-class at trout

Oct 26, 2007, 10:35 AM

Post #3 of 6 (1691 views)
Permalink

Re: Fix for system() returning -1 on dev server [In reply to]

On Fri, Oct 26, 2007 at 01:03:51PM +0930, Jon Schutz wrote:
> That system() returns -1 while running on the Catalyst::Engine::HTTP
> server seems to be a well known fact and was discussed on the list a
> year ago (didn't stop the bug biting me again but).
>
> I have traced the cause to HTTP.pm, line 191 or thereabouts:
>
> local $SIG{CHLD} = 'IGNORE';
>
> Getting rid of this line, or setting $SIG{CHLD} to a proper child reaper
> handler, fixes the problem with system().

Go find the changeset where we switched it to his, and the previous mail
threads, and explain why this won't re-introduce the problems that caused
us to move to IGNORE in the first place.

Also, your patch has no tests.

Both need to happen before it goes into a production release, or at the very
least it needs tests and to be pushed to 5.80.

--
Matt S Trout Need help with your Catalyst or DBIx::Class project?
Technical Director http://www.shadowcat.co.uk/catalyst/
Shadowcat Systems Ltd. Want a managed development or deployment platform?
http://chainsawblues.vox.com/ http://www.shadowcat.co.uk/servers/

_______________________________________________
Catalyst-dev mailing list
Catalyst-dev [at] lists
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

jon+catalyst at youramigo

Oct 26, 2007, 11:22 PM

Post #4 of 6 (1684 views)
Permalink

Re: Fix for system() returning -1 on dev server [In reply to]

On Fri, 2007-10-26 at 18:35 +0100, Matt S Trout wrote:

> Go find the changeset where we switched it to his, and the previous mail
> threads, and explain why this won't re-introduce the problems that caused
> us to move to IGNORE in the first place.
>

It has been IGNORE since SIG{CHLD} was introduced. Defining a child
reaper that just does waitpid is functionally equivalent to using
IGNORE, but apparently IGNORE has an undesirable side-effect on system
().

> Also, your patch has no tests.

optional_http-server.t + live_fork.t already tests for this and was
failing the tests for system() and backticks return codes. Post-patch
it passes system() and still fails backticks exit code.

Since $? may be incorrect as long as $SIG{CHLD} is defined, a code
change more significant than my two-liner would be needed to fix the
backticks result. Backticks still work, it's just the exit code which
is wrong. One could try reaping opportunistically on each request, at
the cost of having zombies hang around for a while on a low-load server,
or double-forking with the first forked child reaped in-line, SIG{CHILD}
being defined in the subprocess to clean up the second fork, and no SIG
{CHLD} in the sub-sub-process. That is a patch for another day.

--

Jon

_______________________________________________
Catalyst-dev mailing list
Catalyst-dev [at] lists
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

andy at hybridized

Oct 27, 2007, 9:46 AM

Post #5 of 6 (1688 views)
Permalink

Re: Fix for system() returning -1 on dev server [In reply to]

On Oct 27, 2007, at 2:22 AM, Jon Schutz wrote:

>> Also, your patch has no tests.
>
> optional_http-server.t + live_fork.t already tests for this and was
> failing the tests for system() and backticks return codes. Post-patch
> it passes system() and still fails backticks exit code.

Hmm, even with the patch both the system and backticks tests fail for
me. The output is the same with or without the patch:

1..13
ok 1 - system
Use of uninitialized value in join or string at t/live_fork.t line 30.
ok 2 - is YAML
not ok 3 - exited OK
# Failed test 'exited OK'
# at t/live_fork.t line 34.
# got: 'No child processes'
# expected: '0'
ok 4 - `backticks`
ok 5 - is YAML
not ok 6 - exited successfully
# Failed test 'exited successfully'
# at t/live_fork.t line 45.
# got: '-1'
# expected: '0'
ok 7 - contains ^/bin/ls$
ok 8 - contains two newlines
ok 9 - fork
ok 10 - is YAML
ok 11 - fork's "pid" wasn't 0
ok 12 - fork got a new pid
ok 13 - fork was effective
# Looks like you failed 2 tests of 13.

FYI for others... in order to run this test you need to comment out
lines 18 and 19 of live_fork.t and run:

TEST_HTTP=1 perl -Ilib t/optional_http-server.t t/live_fork.t

_______________________________________________
Catalyst-dev mailing list
Catalyst-dev [at] lists
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev

jon+catalyst at youramigo

Oct 28, 2007, 12:21 AM

Post #6 of 6 (1684 views)
Permalink

Re: Fix for system() returning -1 on dev server [In reply to]

On Sat, 2007-10-27 at 12:46 -0400, Andy Grundman wrote:

> > optional_http-server.t + live_fork.t already tests for this and was
> > failing the tests for system() and backticks return codes. Post-patch
> > it passes system() and still fails backticks exit code.
>
> Hmm, even with the patch both the system and backticks tests fail for
> me. The output is the same with or without the patch:
>

Well, I checked with vanilla 5.8.8, vanilla 5.9.5 and RHEL5
perl-5.8.8-24, and on all systems I got the same - prepatch both system
() and backticks fail, and postpatch system() is fixed but backticks
still fails.

Nevertheless, I thought I'd have a look at the backticks problem as
well, and implement the double-fork approach - the main process forks
and reaps its child inline, which happens without delay because all the
child does is fork again and exit, so there is no need for any SIG{CHLD}
handling at all.

Attached is suggested patch against svn 7075, with which I get all tests
passing on optional_http-server and optional_http-server-restart. (And
yes, my app still works too!) Please give it a try and see if you have
more success.

--

Jon

Attachments:

catalyst-http-patch-v2.txt (2.40 KB)

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads