Re: patch full of dark magic

On Aug 6, 2010, at 5:42 PM, Jan Dubois wrote:

> Against my better judgment I couldn't prevent myself from peeking at
> the magical patch.  I don't understand what it is doing, and I don't
> have any candies so sacrifice right now either.
> 
> However, I'm always suspicious when I see code like this:
> 
>    if ($^O =~ /win/i) { #Win32 dark magic. It works, so don't change anything
> 
> It isn't clear if that branch should be executed for Cygwin or not (currently
> it will, although the comment sounds like it shouldn't, given that Cygwin is
> considered to be a separate platform from Win32).  I prefer to always make things
> explicit, either
> 
>    if ($^O eq "MSWin32" || $^O eq "cygwin") {
> 
> or
> 
>    if ($^O eq "MSWin32") {

Agreed. I suspect that cygwin uses actual forking, no?

> Of course the "It works, so don't change anything" comment is another alarming
> red flag: you should not make changes to code if you don't understand what the
> changes are doing and/or if you can't explain why it does what it does.  Just
> because you are getting rid of a symptom doesn't mean you solved the real issue.

I *think* it works the way it does because on Win32 it's not really a fork, but a thread, and when you exit in a child thread, it exits the parent, too. Correct?

If so, then the attached patch should work just as well. Alexandr, can you give it a try?

Oh, and BTW, you're my hero for looking into this!

Best,

David

Index: t/16destroy.t
===================================================================
--- t/16destroy.t	(revision 14315)
+++ t/16destroy.t	(working copy)
@@ -76,12 +76,19 @@
     }
 
     sub DESTROY {
-        if ($expect_active < 0) {
-            exit shift->FETCH('Active') || 0;
+        if ($expect_active < 0) { # inside child
+            my $self = shift;
+            exit $self->FETCH('Active') || 0 unless $^O eq 'MSWin32';
+
+            # On Win32, the forked child is actually a thread. So don't exit,
+            # and report failure directly.
+            fail 'Child should be inactive on DESTROY'
+                unless $self->FETCH('Active');
+        } else {
+            return $expect_active
+                ? ok( shift->FETCH('Active'), 'Should be active in DESTROY')
+                : ok( !shift->FETCH('Active'), 'Should not be active in DESTROY');
         }
-        return $expect_active
-            ? ok( shift->FETCH('Active'), 'Should be active in DESTROY')
-            : ok( !shift->FETCH('Active'), 'Should not be active in DESTROY');
     }
 }