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

Commit 9b4eafc

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 meson or 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 This will be backpatched in due course after some testing. Discussion: https://postgr.es/m/20221002164931.d57hlutrcz4d2zi7@awork3.anarazel.de
1 parent fb32748 commit 9b4eafc

File tree

2 files changed

+61
-6
lines changed

2 files changed

+61
-6
lines changed

src/Makefile.global.in

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -482,7 +482,7 @@ cd $(srcdir) && \
482482
TESTLOGDIR='$(CURDIR)/tmp_check/log' \
483483
TESTDATADIR='$(CURDIR)/tmp_check' \
484484
$(with_temp_install) \
485-
PGPORT='6$(DEF_PGPORT)' \
485+
PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' \
486486
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' \
487487
$(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
488488
endef

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

Lines changed: 60 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,21 @@ 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{MESON_BUILD_ROOT}
163+
|| $ENV{top_builddir}
164+
|| $PostgreSQL::Test::Utils::tmp_check ;
165+
$portdir ||= "$build_dir/portlock";
166+
$portdir =~ s!\\!/!g;
167+
# Make sure the directory exists
168+
mkpath($portdir) unless -d $portdir;
151169
}
152170

153171
=pod
@@ -1479,8 +1497,8 @@ start other, non-Postgres servers.
14791497
Ports assigned to existing PostgreSQL::Test::Cluster objects are automatically
14801498
excluded, even if those servers are not currently running.
14811499
1482-
XXX A port available now may become unavailable by the time we start
1483-
the desired service.
1500+
The port number is reserved so that other concurrent test programs will not
1501+
try to use the same port.
14841502
14851503
Note: this is not an instance method. As it's not exported it should be
14861504
called from outside the module as C<PostgreSQL::Test::Cluster::get_free_port()>.
@@ -1532,6 +1550,7 @@ sub get_free_port
15321550
last;
15331551
}
15341552
}
1553+
$found = _reserve_port($port) if $found;
15351554
}
15361555
}
15371556

@@ -1562,6 +1581,40 @@ sub can_bind
15621581
return $ret;
15631582
}
15641583

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

1645+
unlink @port_reservation_files;
1646+
15921647
$? = $exit_code;
15931648
}
15941649

0 commit comments

Comments
 (0)