Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 153e215

Browse files
committed
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 9b4eafc to all live branches Discussion: https://postgr.es/m/20221002164931.d57hlutrcz4d2zi7@awork3.anarazel.de
1 parent 1118a8d commit 153e215

File tree

2 files changed

+60
-5
lines changed

2 files changed

+60
-5
lines changed

src/Makefile.global.in

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -476,6 +476,7 @@ rm -rf '$(CURDIR)'/tmp_check && \
476476
$(MKDIR_P) '$(CURDIR)'/tmp_check && \
477477
cd $(srcdir) && \
478478
TESTDIR='$(CURDIR)' $(with_temp_install) PGPORT='6$(DEF_PGPORT)' \
479+
top_builddir='$(CURDIR)/$(top_builddir)' \
479480
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' \
480481
$(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
481482
endef

src/test/perl/PostgreSQL/Test/Cluster.pm

Lines changed: 59 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -101,9 +101,9 @@ use warnings;
101101

102102
use Carp;
103103
use Config;
104-
use Fcntl qw(:mode);
104+
use Fcntl qw(:mode :flock :seek :DEFAULT);
105105
use File::Basename;
106-
use File::Path qw(rmtree);
106+
use File::Path qw(rmtree mkpath);
107107
use File::Spec;
108108
use File::stat qw(stat);
109109
use File::Temp ();
@@ -117,12 +117,15 @@ use Time::HiRes qw(usleep);
117117
use Scalar::Util qw(blessed);
118118

119119
our ($use_tcp, $test_localhost, $test_pghost, $last_host_assigned,
120-
$last_port_assigned, @all_nodes, $died);
120+
$last_port_assigned, @all_nodes, $died, $portdir);
121121

122122
# the minimum version we believe to be compatible with this package without
123123
# subclassing.
124124
our $min_compat = 12;
125125

126+
# list of file reservations made by get_free_port
127+
my @port_reservation_files;
128+
126129
INIT
127130
{
128131

@@ -148,6 +151,20 @@ INIT
148151

149152
# Tracking of last port value assigned to accelerate free port lookup.
150153
$last_port_assigned = int(rand() * 16384) + 49152;
154+
155+
# Set the port lock directory
156+
157+
# If we're told to use a directory (e.g. from a buildfarm client)
158+
# explicitly, use that
159+
$portdir = $ENV{PG_TEST_PORT_DIR};
160+
# Otherwise, try to use a directory at the top of the build tree
161+
# or as a last resort use the tmp_check directory
162+
my $build_dir = $ENV{top_builddir}
163+
|| $PostgreSQL::Test::Utils::tmp_check ;
164+
$portdir ||= "$build_dir/portlock";
165+
$portdir =~ s!\\!/!g;
166+
# Make sure the directory exists
167+
mkpath($portdir) unless -d $portdir;
151168
}
152169

153170
=pod
@@ -1479,8 +1496,8 @@ start other, non-Postgres servers.
14791496
Ports assigned to existing PostgreSQL::Test::Cluster objects are automatically
14801497
excluded, even if those servers are not currently running.
14811498
1482-
XXX A port available now may become unavailable by the time we start
1483-
the desired service.
1499+
The port number is reserved so that other concurrent test programs will not
1500+
try to use the same port.
14841501
14851502
Note: this is not an instance method. As it's not exported it should be
14861503
called from outside the module as C<PostgreSQL::Test::Cluster::get_free_port()>.
@@ -1532,6 +1549,7 @@ sub get_free_port
15321549
last;
15331550
}
15341551
}
1552+
$found = _reserve_port($port) if $found;
15351553
}
15361554
}
15371555

@@ -1562,6 +1580,40 @@ sub can_bind
15621580
return $ret;
15631581
}
15641582

1583+
# Internal routine to reserve a port number
1584+
# Returns 1 if successful, 0 if port is already reserved.
1585+
sub _reserve_port
1586+
{
1587+
my $port = shift;
1588+
# open in rw mode so we don't have to reopen it and lose the lock
1589+
my $filename = "$portdir/$port.rsv";
1590+
sysopen(my $portfile, $filename, O_RDWR|O_CREAT)
1591+
|| die "opening port file $filename: $!";
1592+
# take an exclusive lock to avoid concurrent access
1593+
flock($portfile, LOCK_EX) || die "locking port file $filename: $!";
1594+
# see if someone else has or had a reservation of this port
1595+
my $pid = <$portfile>;
1596+
chomp $pid;
1597+
if ($pid +0 > 0)
1598+
{
1599+
if (kill 0, $pid)
1600+
{
1601+
# process exists and is owned by us, so we can't reserve this port
1602+
flock($portfile, LOCK_UN);
1603+
close($portfile);
1604+
return 0;
1605+
}
1606+
}
1607+
# All good, go ahead and reserve the port
1608+
seek($portfile, 0, SEEK_SET);
1609+
# print the pid with a fixed width so we don't leave any trailing junk
1610+
print $portfile sprintf("%10d\n",$$);
1611+
flock($portfile, LOCK_UN);
1612+
close($portfile);
1613+
push(@port_reservation_files, $filename);
1614+
return 1;
1615+
}
1616+
15651617
# Automatically shut down any still-running nodes (in the same order the nodes
15661618
# were created in) when the test script exits.
15671619
END
@@ -1582,6 +1634,8 @@ END
15821634
if $exit_code == 0 && PostgreSQL::Test::Utils::all_tests_passing();
15831635
}
15841636

1637+
unlink @port_reservation_files;
1638+
15851639
$? = $exit_code;
15861640
}
15871641

0 commit comments

Comments
 (0)