Test-Stream concurrency, feedback requested

2015-04-27 Thread Chad Granum
I need some feedback on some concurrency related items regarding
Test-Stream.

*What you need to know:*

   -  Test-Simple legacy supported threads, but not forking
   -  Test::SharedFork adds for support, backcompat will be preserved
   -  Test-Stream supports threads and forking
   -  Test-Stream uses a single mechanism for both threading and forking
   (temp files for now, more later)
   -  This email addresses some concurrency edge-cases we need to define or
   preserve behavior for.
   -  I have discovered several bugs in legacy behavior


*What I am looking for in feedback*

   - Have I missed any edge cases?
   - Do the Test-Stream solutions to the edge cases look sane?
   - Do the Test-Stream solution look like they could break anything other
   than existing false-passes (I am ok breaking those)
   - Does anyone have a solution to the thread ending after parent thread
   issue (The only edge-case Test-Stream does not solve, see below)

*Additional discussion*

   - Can/Should Test-Stream wait on all child processes at the end of
   testing? This could break anything that uses Test::SharedFork and
   intentionally leaves a child running after the parent. On the other hand it
   helps prevent one of the edge cases that is otherwise solved by an
   exception.
   - Can/Should Test-Stream wait on all threads at the end of testing?
   Since it appears that perl kills any threads left running, this would only
   have 2 potential consequences: 1) Fix false-passes, 2) Prevent tests from
   ever finishing if they currently have threads with infinite loops. The
   benefit is that is solves the only remaining thread edge case that
   Test-Stream has not fixed.
   - The concurrency support must go out with Test-Stream, it is needed for
   legacy thread support and Test::SharedFork support. That said should it be
   a private and unpublished API for now that gets unlocked later when we like
   how it works, or can it be published with initial release?


*The data!*

I have written several scripts that demonstrate these edge cases. Each
script has a stream and legacy variant, the difference is minimal in each
case. I have added the output from each script to the script under __END__
for easy reference.

Files can be found here:
https://github.com/Test-More/test-more/tree/stream/fix_concurrency/concurrency_demos

Here is my list of edge cases, as well as how legacy and stream handle
them. When I mention forking in legacy it is with Test::SharedFork loaded.

*These just work, when done right.*
*Forking or threading inside a subtest*

   - Works fine in both so long as you remember to exit/wait/join in the
   right places

*Forking or threading without subtests*

   - Works fine in both as long as you remember to exit/wait/join in the
   right places


*The non-subtest Edge Cases:*

*Parent ends BEFORE the child process*

   - In legacy, test passes as though nothing were wrong, there is an
   error, but the harness reports success
   - In Stream, initial tap passes, but a helpful message is displayed and
   an extra 'not ok' is sent to stdout to poison the TAP so that the harness
   reports failure.

*Parent ends BEFORE the thread finishes (not solvable?)*

   - Due to the way perl threads works there is no way I can find for a
   thread to announce that this issue happened.
   - Both legacy and stream fail to alert the harness that anything is
   wrong, resulting in a false pass.
   - There is some output from perl about threads still running, but this
   does not cause a failure.

*Subtest related edge cases:*

*Forking or threading and then starting a subtest*

   - No issues in Test-Stream, it just works
   - Fails in legacy as the plan does not include the subtest, otherwise
   TAP output looks fine.
   - Test::SharedFork and thread support both have the same bug.

*Failing to join a thread before the subtest completes in the parent*

   - In Test-Stream this throws an exception and the harness catches the
   failure
   - In Legacy this would output garbled subtest tap, but falsely report
   success!

*Forgetting to exit the process after forking inside a subtest*

   - In Test-Stream this throws an exception, harness catches it, useful
   message is seen, no false positives.
   - In Legacy this outputs garbled tap, the harness catches it because
   some test numbers are duplicated and the plan is wrong. If there is no plan
   and numbers are disabled it might be a false pass.

*Letting the subtest finish in the parent while a child is sending it
events*

   - Legacy has garbled subtest TAP, may miss failures and report a false
   pass.
   - Test-Stream throws an exception with a useful message, harness notices
   the failure so there is no false-pass.

*Forgetting to exit the child process and failing to wait on it (in
subtest)*

   - Legacy may report false pass if numbers are disabled and there is no
   plan. Generally though the failure is caught by accident
   - Test-Stream alerts you to both issues, 

Re: File::Temp/File::Spec problems on Windows under taint mode – breaking change needed?

2015-04-27 Thread Jens Rehsack

 Am 24.04.2015 um 05:37 schrieb Aristotle Pagaltzis pagalt...@gmx.de:
 
 * David Golden x...@xdg.me [2015-04-23 17:40]:
 [...]
 Please see https://rt.cpan.org/Ticket/Display.html?id=60340 for context.
 
 I think File::Temp needs to be able to work around File::Spec::Win32
 returning a non-writable directory.
 
 My proposal was to warn and fall back to ..  That's a small breaking
 change, but I think doing something in a different place than
 requested is better than failing entirely.
 
 Alternatively, it needs to validate the Win32 response and throw an error
 early, before attempting to make the directory so that the error message is
 more informative.
 
 Thoughts?
 
 Windows considers the current directory an implied part of %PATH%,

True

 has no concept like the executable bit of Unixoid OSes,

Nope - it has such a concept, but it's in ACL's, not in attributes. History of
Windows was non-multi-user *shrug* - weird implementation, crummy supported ;)

 nor does it allow
 unlinking files while they’re open.

Depends on Filesystem ...

 So I’d feel uncomfortable about just
 unexpectedly dropping junk into the current directory, announced or not.
 Therefore I’d tend toward the latter.

Is there a reason not to tryout Win32 API function GetTempPath 
(https://msdn.microsoft.com/en-us/library/windows/desktop/aa364992%28v=vs.85%29.aspx)?
I dunno how File::Spec/File::Temp on Unix behaves tmp points to a non-writable 
directory (think r/o file-system).

 But someone with better instincts for Windows may be able to call this
 bunk. Mostly I didn’t want to leave the question warnocked.

I agree - '.' is the worst choice one can make - and I intend to say 'start 
again with TMP=.' or so.

Cheers
-- 
Jens Rehsack
rehs...@gmail.com



Re: File::Temp/File::Spec problems on Windows under taint mode – breaking change needed?

2015-04-27 Thread David Golden
On Mon, Apr 27, 2015 at 4:10 PM, Jens Rehsack rehs...@gmail.com wrote:

 Is there a reason not to tryout Win32 API function GetTempPath (
 https://msdn.microsoft.com/en-us/library/windows/desktop/aa364992%28v=vs.85%29.aspx
 )?
 I dunno how File::Spec/File::Temp on Unix behaves tmp points to a
 non-writable directory (think r/o file-system).


Good point.  I think that's feasible if Win32::API is installed, which most
Win32 Perl's should have, IIRC.  As a further fallback, I think all Win32
Perl's ship with Win32.pm, which gives Win32::GetFolderPath.  Then
CSIDL_INTERNET_CACHE
looks like the best choice for temporary data.

David


-- 
David Golden x...@xdg.me Twitter/IRC: @xdg