Guard %ENV access with exists() checks and some minor edge case bugs I found while doing so

lib/Test/Tester.pm

@@ -13,6 +13,7 @@ BEGIN
 use Test::Builder;
 use Test::Tester::CaptureRunner;
 use Test::Tester::Delegate;
+use Test2::Util qw(_env_get);
 
 require Exporter;
 
@@ -30,7 +31,7 @@ $Delegator->{Object} = $Test;
 
 my $runner = Test::Tester::CaptureRunner->new;
 
-my $want_space = $ENV{TESTTESTERSPACE};
+my $want_space = _env_get('TESTTESTERSPACE');
 
 sub show_space
 {
@@ -40,7 +41,7 @@ sub show_space
 my $colour = '';
 my $reset = '';
 
-if (my $want_colour = $ENV{TESTTESTERCOLOUR} || $ENV{TESTTESTERCOLOR})
+if (my $want_colour = _env_get('TESTTESTERCOLOUR') || _env_get('TESTTESTERCOLOR'))
 {
 	if (eval { require Term::ANSIColor; 1 })
 	{

lib/Test2/API.pm

@@ -3,7 +3,7 @@ use strict;
 use warnings;
 
 use Time::HiRes qw/time/;
-use Test2::Util qw/USE_THREADS/;
+use Test2::Util qw/USE_THREADS _env_get/;
 
 BEGIN {
     $ENV{TEST_ACTIVE} ||= 1;
@@ -287,7 +287,8 @@ sub test2_ipc_get_timeout     { $INST->ipc_timeout() }
 sub test2_ipc_enable_shm      { 0 }
 
 sub test2_formatter     {
-    if ($ENV{T2_FORMATTER} && $ENV{T2_FORMATTER} =~ m/^(\+)?(.*)$/) {
+    my $env_var = _env_get('T2_FORMATTER', '');
+    if ($env_var =~ m/^(\+)?(.+)$/) {
         my $formatter = $1 ? $2 : "Test2::Formatter::$2";
         my $file = pkg_to_file($formatter);
         require $file;

lib/Test2/API/Instance.pm

@@ -8,7 +8,7 @@ our @CARP_NOT = qw/Test2::API Test2::API::Instance Test2::IPC::Driver Test2::For
 use Carp qw/confess carp/;
 use Scalar::Util qw/reftype/;
 
-use Test2::Util qw/get_tid USE_THREADS CAN_FORK pkg_to_file try CAN_SIGSYS/;
+use Test2::Util qw/get_tid USE_THREADS CAN_FORK pkg_to_file try CAN_SIGSYS _env_get/;
 
 use Test2::EventFacet::Trace();
 use Test2::API::Stack();
@@ -104,7 +104,7 @@ sub post_preload_reset {
 
     $self->{+FINALIZED} = undef;
     $self->{+IPC}       = undef;
-    $self->{+IPC_DISABLED} = $ENV{T2_NO_IPC} ? 1 : 0;
+    $self->{+IPC_DISABLED} = _env_get('T2_NO_IPC') ? 1 : 0;
 
     $self->{+IPC_TIMEOUT} = DEFAULT_IPC_TIMEOUT() unless defined $self->{+IPC_TIMEOUT};
 
@@ -131,7 +131,7 @@ sub reset {
 
     $self->{+FINALIZED}    = undef;
     $self->{+IPC}          = undef;
-    $self->{+IPC_DISABLED} = $ENV{T2_NO_IPC} ? 1 : 0;
+    $self->{+IPC_DISABLED} = _env_get('T2_NO_IPC') ? 1 : 0;
 
     $self->{+IPC_TIMEOUT} = DEFAULT_IPC_TIMEOUT() unless defined $self->{+IPC_TIMEOUT};
 
@@ -163,10 +163,10 @@ sub _finalize {
 
     unless ($self->{+FORMATTER}) {
         my ($formatter, $source);
-        if ($ENV{T2_FORMATTER}) {
+        if (my $env_val = _env_get('T2_FORMATTER')) {
             $source = "set by the 'T2_FORMATTER' environment variable";
 
-            if ($ENV{T2_FORMATTER} =~ m/^(\+)?(.*)$/) {
+            if ($env_val =~ m/^(\+)?(.+)$/) {
                 $formatter = $1 ? $2 : "Test2::Formatter::$2"
             }
             else {
@@ -177,7 +177,8 @@ sub _finalize {
             ($formatter) = @{$self->{+FORMATTERS}};
             $source = "Most recently added";
         }
-        else {
+
+        if (!$formatter) {
             $formatter = 'Test2::Formatter::TAP';
             $source    = 'default formatter';
         }

lib/Test2/Event.pm

@@ -9,7 +9,7 @@ use Carp qw/croak/;
 
 use Test2::Util::HashBase qw/trace -amnesty uuid -_eid -hubs/;
 use Test2::Util::ExternalMeta qw/meta get_meta set_meta delete_meta/;
-use Test2::Util qw/pkg_to_file gen_uid/;
+use Test2::Util qw/pkg_to_file gen_uid _env_get/;
 
 use Test2::EventFacet::About();
 use Test2::EventFacet::Amnesty();
@@ -294,7 +294,7 @@ sub nested {
     my $self = shift;
 
     Carp::cluck("Use of Test2::Event->nested() is deprecated, use Test2::Event->trace->nested instead")
-        if $ENV{AUTHOR_TESTING};
+        if _env_get('AUTHOR_TESTING');
 
     if (my $hubs = $self->{+HUBS}) {
         return $hubs->[0]->{nested} if @$hubs;
@@ -308,7 +308,7 @@ sub in_subtest {
     my $self = shift;
 
     Carp::cluck("Use of Test2::Event->in_subtest() is deprecated, use Test2::Event->trace->hid instead")
-        if $ENV{AUTHOR_TESTING};
+        if _env_get('AUTHOR_TESTING');
 
     my $hubs = $self->{+HUBS};
     if ($hubs && @$hubs) {

lib/Test2/Formatter/TAP.pm

@@ -4,7 +4,7 @@ use warnings;
 
 our $VERSION = '1.302195';
 
-use Test2::Util qw/clone_io/;
+use Test2::Util qw/clone_io _env_get/;
 
 use Test2::Util::HashBase qw{
     no_numbers handles _encoding _last_fh
@@ -116,7 +116,7 @@ sub write {
         my $io = $handles->[$hid] or next;
 
         print $io "\n"
-            if $ENV{HARNESS_ACTIVE}
+            if _env_get('HARNESS_ACTIVE')
             && $hid == OUT_ERR
             && $self->{+_LAST_FH} != $io
             && $msg =~ m/^#\s*Failed( \(TODO\))? test /;
@@ -294,7 +294,7 @@ sub assert_tap {
         # In a verbose harness we indent the extra since they will appear
         # inside the subtest braces. This helps readability. In a non-verbose
         # harness we do not do this because it is less readable.
-        if ($ENV{HARNESS_IS_VERBOSE} || !$ENV{HARNESS_ACTIVE}) {
+        if (_env_get("HARNESS_IS_VERBOSE") || !_env_get("HARNESS_ACTIVE")) {
             $extra_indent = "    ";
             $extra_space = ' ';
         }

lib/Test2/IPC/Driver/Files.pm

@@ -14,7 +14,7 @@ use Storable();
 use File::Spec();
 use POSIX();
 
-use Test2::Util qw/try get_tid pkg_to_file IS_WIN32 ipc_separator do_rename do_unlink try_sig_mask/;
+use Test2::Util qw/try get_tid pkg_to_file IS_WIN32 ipc_separator do_rename do_unlink try_sig_mask _env_get/;
 use Test2::API qw/test2_ipc_set_pending/;
 
 sub is_viable { 1 }
@@ -23,7 +23,7 @@ sub init {
     my $self = shift;
 
     my $tmpdir = File::Temp::tempdir(
-        $ENV{T2_TEMPDIR_TEMPLATE} || "test2" . ipc_separator . $$ . ipc_separator . "XXXXXX",
+        _env_get('T2_TEMPDIR_TEMPLATE') || "test2" . ipc_separator . $$ . ipc_separator . "XXXXXX",
         CLEANUP => 0,
         TMPDIR => 1,
     );
@@ -33,7 +33,7 @@ sub init {
     $self->{+TEMPDIR} = File::Spec->canonpath($tmpdir);
 
     print STDERR "\nIPC Temp Dir: $tmpdir\n\n"
-        if $ENV{T2_KEEP_TEMPDIR};
+        if _env_get('T2_KEEP_TEMPDIR');
 
     $self->{+EVENT_IDS} = {};
     $self->{+READ_IDS} = {};
@@ -107,7 +107,7 @@ sub drop_hub {
     $self->abort_trace("A hub file can only be closed by the thread that started it\nExpected $tid, got " . get_tid())
         unless get_tid() == $tid;
 
-    if ($ENV{T2_KEEP_TEMPDIR}) {
+    if (_env_get('T2_KEEP_TEMPDIR')) {
         my ($ok, $err) = do_rename($hfile, File::Spec->canonpath("$hfile.complete"));
         $self->abort_trace("Could not rename file '$hfile' -> '$hfile.complete': $err") unless $ok
     }
@@ -266,7 +266,7 @@ sub cull {
         # Do not remove global events
         next if $info->{global};
 
-        if ($ENV{T2_KEEP_TEMPDIR}) {
+        if (_env_get('T2_KEEP_TEMPDIR')) {
             my $complete = File::Spec->canonpath("$full.complete");
             my ($ok, $err) = do_rename($full, $complete);
             $self->abort("Could not rename IPC file '$full', '$complete': $err") unless $ok;
@@ -406,7 +406,7 @@ sub DESTROY {
         if ($aborted || $file =~ m/^(GLOBAL|HUB$sep)/) {
             $full =~ m/^(.*)$/;
             $full = $1; # Untaint it
-            next if $ENV{T2_KEEP_TEMPDIR};
+            next if _env_get('T2_KEEP_TEMPDIR');
             my ($ok, $err) = do_unlink($full);
             $self->abort("Could not unlink IPC file '$full': $err") unless $ok;
             next;
@@ -416,7 +416,7 @@ sub DESTROY {
     }
     closedir($dh);
 
-    if ($ENV{T2_KEEP_TEMPDIR}) {
+    if (_env_get('T2_KEEP_TEMPDIR')) {
         print STDERR "# Not removing temp dir: $tempdir\n";
         return;
     }

lib/Test2/Util.pm

@@ -36,13 +36,26 @@ our @EXPORT_OK = qw{
     try_sig_mask
 
     clone_io
+
+    _env_get
 };
 BEGIN { require Exporter; our @ISA = qw(Exporter) }
 
 BEGIN {
     *IS_WIN32 = ($^O eq 'MSWin32') ? sub() { 1 } : sub() { 0 };
 }
 
+# check for key existence before fetching from %ENV to avoid
+# locked hash issues. If it does not exist, or it does and the
+# value is undefined return $_[1] instead, thus allowing
+# a default value to be supplied if required.
+sub _env_get {
+    my ($key,$default) = @_;
+    my $got = exists($ENV{$key}) ? $ENV{$key} : undef;
+    $got = $default unless defined $got;
+    return $got;
+}
+
 sub _can_thread {
     return 0 unless $] >= 5.008001;
     return 0 unless $Config{'useithreads'};