Prevent port collisions between concurrent TAP tests

Currently there is a race condition where if concurrent TAP tests both
test that they can open a port they will assume that it is free and use
it, causing one of them to fail. To prevent this we record a reservation
using an exclusive lock, and any TAP test that discovers a reservation
checks to see if the reserving process is still alive, and looks for
another free port if it is.

Ports are reserved in a directory set by the environment setting
PG_TEST_PORT_DIR, or if that doesn't exist a subdirectory of the top
build directory as set by Makefile.global, or its own
tmp_check directory.

The prove_check recipe in Makefile.global.in is extended to export
top_builddir to the TAP tests. This was already exported by the
prove_installcheck recipes.

Per complaint from Andres Freund

Backpatched from 9b4eafcaf4 to all live branches

Discussion: https://postgr.es/m/20221002164931.d57hlutrcz4d2zi7@awork3.anarazel.de

diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index d8b8dd9117bc0f9961e1c6bc8aedaef35105e28f..27f8342861f0e11f09deac2e4cc496cb73f609f6 100644 (file)
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -455,6 +455,7 @@ rm -rf '$(CURDIR)'/tmp_check
 $(MKDIR_P) '$(CURDIR)'/tmp_check
 cd $(srcdir) && \
    TESTDIR='$(CURDIR)' $(with_temp_install) PGPORT='6$(DEF_PGPORT)' \
+   top_builddir='$(CURDIR)/$(top_builddir)' \
    PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' \
    $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
 endef

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index a15f2c7d25b1e70470c92f282b48287cf35f6b9d..506d39a3c33eb2605c7af9a6126b13b51f43dcc0 100644 (file)
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -90,9 +90,9 @@ use Carp;
 use Config;
 use Cwd;
 use Exporter 'import';
-use Fcntl qw(:mode);
+use Fcntl qw(:mode :flock :seek :DEFAULT);
 use File::Basename;
-use File::Path qw(rmtree);
+use File::Path qw(rmtree mkpath);
 use File::Spec;
 use File::stat qw(stat);
 use File::Temp ();
@@ -110,7 +110,10 @@ our @EXPORT = qw(
 );
 
 our ($use_tcp, $test_localhost, $test_pghost, $last_host_assigned,
-   $last_port_assigned, @all_nodes, $died);
+   $last_port_assigned, @all_nodes, $died, $portdir);
+
+# list of file reservations made by get_free_port
+my @port_reservation_files;
 
 # For backward compatibility only.
 our $vfs_path = '';
@@ -134,6 +137,20 @@ INIT
 
    # Tracking of last port value assigned to accelerate free port lookup.
    $last_port_assigned = int(rand() * 16384) + 49152;
+
+   # Set the port lock directory
+
+   # If we're told to use a directory (e.g. from a buildfarm client)
+   # explicitly, use that
+   $portdir = $ENV{PG_TEST_PORT_DIR};
+   # Otherwise, try to use a directory at the top of the build tree
+   # or as a last resort use the tmp_check directory
+   my $build_dir = $ENV{top_builddir}
+     || $TestLib::tmp_check ;
+   $portdir ||= "$build_dir/portlock";
+   $portdir =~ s!\\!/!g;
+   # Make sure the directory exists
+   mkpath($portdir) unless -d $portdir;
 }
 
 =pod
@@ -1133,8 +1150,8 @@ by test cases that need to start other, non-Postgres servers.
 Ports assigned to existing PostgresNode objects are automatically
 excluded, even if those servers are not currently running.
 
-XXX A port available now may become unavailable by the time we start
-the desired service.
+The port number is reserved so that other concurrent test programs will not
+try to use the same port.
 
 =cut
 
@@ -1183,6 +1200,7 @@ sub get_free_port
                    last;
                }
            }
+           $found = _reserve_port($port) if $found;
        }
    }
 
@@ -1213,6 +1231,40 @@ sub can_bind
    return $ret;
 }
 
+# Internal routine to reserve a port number
+# Returns 1 if successful, 0 if port is already reserved.
+sub _reserve_port
+{
+   my $port = shift;
+   # open in rw mode so we don't have to reopen it and lose the lock
+   my $filename = "$portdir/$port.rsv";
+   sysopen(my $portfile, $filename, O_RDWR|O_CREAT)
+     || die "opening port file $filename: $!";
+   # take an exclusive lock to avoid concurrent access
+   flock($portfile, LOCK_EX) || die "locking port file $filename: $!";
+   # see if someone else has or had a reservation of this port
+   my $pid = <$portfile>;
+   chomp $pid;
+   if ($pid +0 > 0)
+   {
+       if (kill 0, $pid)
+       {
+           # process exists and is owned by us, so we can't reserve this port
+           flock($portfile, LOCK_UN);
+           close($portfile);
+           return 0;
+       }
+   }
+   # All good, go ahead and reserve the port
+   seek($portfile, 0, SEEK_SET);
+   # print the pid with a fixed width so we don't leave any trailing junk
+   print $portfile sprintf("%10d\n",$$);
+   flock($portfile, LOCK_UN);
+   close($portfile);
+   push(@port_reservation_files, $filename);
+   return 1;
+}
+
 # Automatically shut down any still-running nodes when the test script exits.
 # Note that this just stops the postmasters (in the same order the nodes were
 # created in).  Any temporary directories are deleted, in an unspecified
@@ -1234,6 +1286,8 @@ END
        $node->clean_node if $exit_code == 0 && TestLib::all_tests_passing();
    }
 
+   unlink @port_reservation_files;
+
    $? = $exit_code;
 }