Skip to content

Commit

Permalink
Don't cache the socket-IO while connection setup
Browse files Browse the repository at this point in the history
The file_no of the socket IO can change while connecting.
This can happen when alternative hosts are tried,
while GSS authentication
and when falling back to unencrypted in sslmode:prefer .
Therefore expire the socket IO at each connect_poll and reset_poll call.

Caching the IO previosly led to occasional errors kind of:
  Errno::EBADF: Bad file descriptor
  • Loading branch information
larskanis committed Mar 9, 2022
1 parent f270b71 commit b88d59b
Show file tree
Hide file tree
Showing 5 changed files with 35 additions and 19 deletions.
8 changes: 2 additions & 6 deletions ext/pg_connection.c
Original file line number Diff line number Diff line change
Expand Up @@ -475,9 +475,7 @@ pgconn_connect_poll(VALUE self)
PostgresPollingStatusType status;
status = gvl_PQconnectPoll(pg_get_pgconn(self));

if ( status == PGRES_POLLING_FAILED ) {
pgconn_close_socket_io(self);
}
pgconn_close_socket_io(self);

return INT2FIX((int)status);
}
Expand Down Expand Up @@ -556,9 +554,7 @@ pgconn_reset_poll(VALUE self)
PostgresPollingStatusType status;
status = gvl_PQresetPoll(pg_get_pgconn(self));

if ( status == PGRES_POLLING_FAILED ) {
pgconn_close_socket_io(self);
}
pgconn_close_socket_io(self);

return INT2FIX((int)status);
}
Expand Down
7 changes: 2 additions & 5 deletions lib/pg/connection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -612,9 +612,6 @@ def cancel
alias async_cancel cancel

private def async_connect_or_reset(poll_meth)
# Now grab a reference to the underlying socket so we know when the connection is established
socket = socket_io

# Track the progress of the connection, waiting for the socket to become readable/writable before polling it
poll_status = PG::PGRES_POLLING_WRITING
until poll_status == PG::PGRES_POLLING_OK ||
Expand All @@ -623,11 +620,11 @@ def cancel
# If the socket needs to read, wait 'til it becomes readable to poll again
case poll_status
when PG::PGRES_POLLING_READING
socket.wait_readable
socket_io.wait_readable

# ...and the same for when the socket needs to write
when PG::PGRES_POLLING_WRITING
socket.wait_writable
socket_io.wait_writable
end

# Check to see if it's finished or failed yet
Expand Down
10 changes: 3 additions & 7 deletions sample/async_api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,6 @@ def output_progress( msg )
abort "Connection failed: %s" % [ conn.error_message ] if
conn.status == PG::CONNECTION_BAD

# Now grab a reference to the underlying socket so we know when the
# connection is established
socket = conn.socket_io

# Track the progress of the connection, waiting for the socket to become readable/writable
# before polling it
poll_status = PG::PGRES_POLLING_WRITING
Expand All @@ -41,13 +37,13 @@ def output_progress( msg )
case poll_status
when PG::PGRES_POLLING_READING
output_progress " waiting for socket to become readable"
select( [socket], nil, nil, TIMEOUT ) or
select( [conn.socket_io], nil, nil, TIMEOUT ) or
raise "Asynchronous connection timed out!"

# ...and the same for when the socket needs to write
when PG::PGRES_POLLING_WRITING
output_progress " waiting for socket to become writable"
select( nil, [socket], nil, TIMEOUT ) or
select( nil, [conn.socket_io], nil, TIMEOUT ) or
raise "Asynchronous connection timed out!"
end

Expand Down Expand Up @@ -85,7 +81,7 @@ def output_progress( msg )
# Buffer any incoming data on the socket until a full result is ready.
conn.consume_input
while conn.is_busy
select( [socket], nil, nil, TIMEOUT ) or
select( [conn.socket_io], nil, nil, TIMEOUT ) or
raise "Timeout waiting for query response."
conn.consume_input
end
Expand Down
2 changes: 1 addition & 1 deletion spec/helpers/tcp_gate_scheduler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ def io_wait(io, events, duration)

# compare and store the fileno for debugging
if conn.observed_fd && conn.observed_fd != io.fileno
raise "observed fd changed: old:#{conn.observed_fd} new:#{io.fileno}"
puts "observed fd changed: old:#{conn.observed_fd} new:#{io.fileno}"
end
conn.observed_fd = io.fileno

Expand Down
27 changes: 27 additions & 0 deletions spec/pg/connection_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,33 @@
res = @conn2.query("SELECT 4")
end

it "can work with changing IO while connection setup" do
# The file_no of the socket IO can change while connecting.
# This can happen when alternative hosts are tried,
# while GSS authentication
# and when falling back to unencrypted in sslmode:prefer

# Consume some file descriptors and free them while the connection is established.
pipes = 100.times.map{ IO.pipe }
Thread.new do
pipes.reverse_each do |ios|
ios.each(&:close)
sleep 0.01
end
end

# Connect with SSL, but use a wrong client cert, so that connection is aborted.
# A second connection is then started with a new IO.
# And since the pipes above were freed in the concurrent thread above, there is a high chance that it's a lower file descriptor than before.
conn = PG.connect( @conninfo + " sslcert=tmp_test_specs/data/ruby-pg-ca-cert" )

# The new connection should work even when the file descriptor has changed.
res = conn.exec("SELECT 1")
expect( res.values ).to eq([["1"]])

conn.close
end

it "can use conn.reset_start to restart the connection" do
ios = IO.pipe
conn = described_class.connect_start( @conninfo )
Expand Down

0 comments on commit b88d59b

Please sign in to comment.