8000 Prevent port collisions between concurrent TAP tests · postgres/postgres@46def52 · GitHub
[go: up one dir, main page]
Skip to content

Commit 46def52

Browse files
adunstanadunstan
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 aa1fe77 commit 46def52

File tree

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
@@ -455,6 +455,7 @@ rm -rf '$(CURDIR)'/tmp_check
455455
$(MKDIR_P) '$(CURDIR)'/tmp_check
456456
cd $(srcdir) && \
457457
TESTDIR='$(CURDIR)' $(with_temp_install) PGPORT='6$(DEF_PGPORT)' \
458+
top_builddir='$(CURDIR)/$(top_builddir)' \
458459
PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' \
459460
$(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
460461
endef

‎src/test/perl/PostgresNode.pm

Lines changed: 59 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -90,9 +90,9 @@ use Carp;
9090
use Config;
9191
use Cwd;
9292
use Exporter 'import';
93-
use Fcntl qw(:mode);
93+
use Fcntl qw(:mode :flock :seek :DEFAULT);
9494
use File::Basename;
95-
use File::Path qw(rmtree);
95+
use File::Path qw(rmtree mkpath);
9696
use File::Spec;
9797
use File::stat qw(stat);
9898
use File::Temp ();
@@ -110,7 +110,10 @@ our @EXPORT = qw(
110110
);
111111

112112
our ($use_tcp, $test_localhost, $test_pghost, $last_host_assigned,
113-
$last_port_assigned, @all_nodes, $died);
113+
$last_port_assigned, @all_nodes, $died, $portdir);
114+
115+
# list of file reservations made by get_free_port
116+
my @port_reservation_files;
114117

115118
# For backward compatibility only.
116119
our $vfs_path = '';
@@ -134,6 +137,20 @@ INIT
134137

135138
# Tracking of last port value assigned to accelerate free port lookup.
136139
$last_port_assigned = int(rand() * 16384) + 49152;
140+
141+
# Set the port lock directory
142+
143+
# If we're told to use a directory (e.g. from a buildfarm client)
144+
# explicitly, use that
145+
$portdir = $ENV{PG_TEST_PORT_DIR};
146+
# Otherwise, try to use a directory at the top of the build tree
147+
# or as a last resort use the tmp_check directory
148+
my $build_dir = $ENV{top_builddir}
149+
|| $TestLib::tmp_check ;
150+
$portdir ||= "$build_dir/portlock";
151+
$portdir =~ s!\\!/!g;
152+
# Make sure the directory exists
153+
mkpath($portdir) unless -d $portdir;
137154
}
138155

139156
=pod
@@ -1133,8 +1150,8 @@ by test cases that need to start other, non-Postgres servers.
11331150
Ports assigned to existing PostgresNode objects are automatically
11341151
excluded, even if those servers are not currently running.
11351152
1136-
XXX A port available now may become unavailable by the time we start
1137-
the desired service.
1153+
The port number is reserved so that other concurrent test programs will not
1154+
try to use the same port.
11381155
11391156
=cut
11401157

@@ -1183,6 +1200,7 @@ sub get_free_port
11831200
last;
11841201
}
11851202
}
1203+
$found = _reserve_port($port) if $found;
11861204
}
11871205
}
11881206

@@ -1213,6 +1231,40 @@ sub can_bind
12131231
return $ret;
12141232
}
12151233

1234+
# Internal routine to reserve a port number
1235+
# Returns 1 if successful, 0 if port is already reserved.
1236+
sub _reserve_port
1237+
{
1238+
my $port = shift;
1239+
# open in rw mode so we don't have to reopen it and lose the lock
1240+
my $filename = "$portdir/$port.rsv";
1241+
sysopen(my $portfile, $filename, O_RDWR|O_CREAT)
1242+
|| die "opening port file $filename: $!";
1243+
# take an exclusive lock to avoid concurrent access
1244+
flock($portfile, LOCK_EX) || die "locking port file $filename: $!";
1245+
# see if someone else has or had a reservation of this port
1246+
my $pid = <$portfile>;
1247+
chomp $pid;
1248+
if ($pid +0 > 0)
1249+
{
1250+
if (kill 0, $pid)
1251+
{
1252+
# process exists and is owned by us, so we can't reserve this port
1253+
flock($portfile, LOCK_UN);
1254+
close($portfile);
1255+
return 0;
1256+
}
1257+
}
1258+
# All good, go ahead and reserve the port
1259+
seek($portfile, 0, SEEK_SET);
1260+
# print the pid with a fixed width so we don't leave any trailing junk
1261+
print $portfile sprintf("%10d\n",$$);
1262+
flock($portfile, LOCK_UN);
1263+
close($portfile);
1264+
push(@port_reservation_files, $filename);
1265+
return 1;
1266+
}
1267+
12161268< 67F4 /td>
# Automatically shut down any still-running nodes when the test script exits.
12171269
# Note that this just stops the postmasters (in the same order the nodes were
12181270
# created in). Any temporary directories are deleted, in an unspecified
@@ -1234,6 +1286,8 @@ END
12341286
$node->clean_node if $exit_code == 0 && TestLib::all_tests_passing();
12351287
}
12361288

1289+
unlink @port_reservation_files;
1290+
12371291
$? = $exit_code;
12381292
}
12391293

0 commit comments

Comments
 (0)
0