Skip to content

Commit 538b8b6

Browse files
authored
Fix out_forward keepalive reuse after remote close (#5309)
**Which issue(s) this PR fixes**: Fixes #5269 Fixes #4618 **What this PR does / why we need it**: This PR fixes a keepalive socket reuse bug in `out_forward`. When a cached keepalive connection has already been closed by the remote side, `out_forward` could pick that socket back up and try to write to it again. In the reported TLS/NLB setup, that leaves the flush thread spinning on a dead socket and can drive CPU usage to 100%. The fix is to validate cached sockets before reusing them. If a socket has already become unreadable, closed, or otherwise looks no longer safe to reuse, it is discarded and a new connection is created instead. This keeps `out_forward` from reusing half-closed keepalive sockets and avoids the busy-loop behavior described in the issue. This PR also adds regression coverage to make sure a socket that was closed by the peer is not reused from the keepalive cache. **Docs Changes**: None. **Release Note**: - out_forward: avoid reusing closed keepalive sockets after remote disconnects Signed-off-by: aviralgarg05 <gargaviral99@gmail.com>
1 parent 6a27913 commit 538b8b6

2 files changed

Lines changed: 78 additions & 10 deletions

File tree

lib/fluent/plugin/out_forward/socket_cache.rb

Lines changed: 45 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -31,20 +31,26 @@ def initialize(timeout, log)
3131
end
3232

3333
def checkout_or(key)
34+
obsolete_sockets = []
35+
3436
@mutex.synchronize do
35-
tsock = pick_socket(key)
37+
tsock, obsolete_sockets = pick_socket(key)
3638

3739
if tsock
38-
tsock.sock
40+
return tsock.sock
3941
else
4042
sock = yield
4143
new_tsock = TimedSocket.new(timeout, key, sock)
4244
@log.debug("connect new socket #{new_tsock}")
4345

4446
@inflight_sockets[sock] = new_tsock
45-
new_tsock.sock
47+
return new_tsock.sock
4648
end
4749
end
50+
ensure
51+
obsolete_sockets.each do |sock|
52+
sock.sock.close rescue nil
53+
end
4854
end
4955

5056
def checkin(sock)
@@ -117,17 +123,32 @@ def clear
117123
# this method is not thread safe
118124
def pick_socket(key)
119125
if @available_sockets[key].empty?
120-
return nil
126+
return nil, []
121127
end
122128

123129
t = Time.now
124-
if (s = @available_sockets[key].find { |sock| !expired_socket?(sock, time: t) })
125-
@inflight_sockets[s.sock] = @available_sockets[key].delete(s)
126-
s.timeout = timeout
127-
s
128-
else
129-
nil
130+
selected = nil
131+
remaining = []
132+
obsolete_sockets = []
133+
134+
@available_sockets[key].each do |sock|
135+
if expired_socket?(sock, time: t) || unavailable_socket?(sock.sock)
136+
obsolete_sockets << sock
137+
elsif selected.nil?
138+
selected = sock
139+
else
140+
remaining << sock
141+
end
142+
end
143+
144+
@available_sockets[key] = remaining
145+
146+
if selected
147+
@inflight_sockets[selected.sock] = selected
148+
selected.timeout = timeout
130149
end
150+
151+
[selected, obsolete_sockets]
131152
end
132153

133154
def timeout
@@ -137,6 +158,20 @@ def timeout
137158
def expired_socket?(sock, time: Time.now)
138159
sock.timeout ? sock.timeout < time : false
139160
end
161+
162+
def unavailable_socket?(sock)
163+
return sock.closed? if sock.respond_to?(:closed?) && sock.closed?
164+
165+
io = if sock.respond_to?(:to_io)
166+
sock.to_io
167+
elsif sock.is_a?(IO)
168+
sock
169+
end
170+
171+
io ? !!IO.select([io], nil, nil, 0) : false
172+
rescue IOError, SystemCallError
173+
true
174+
end
140175
end
141176
end
142177
end

test/plugin/out_forward/test_socket_cache.rb

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
require 'fluent/plugin/out_forward/socket_cache'
44
require 'timecop'
5+
require 'socket'
56

67
class SocketCacheTest < Test::Unit::TestCase
78
sub_test_case 'checkout_or' do
@@ -48,6 +49,38 @@ class SocketCacheTest < Test::Unit::TestCase
4849
c.checkout_or('key') { 'new socket' }
4950
end
5051
end
52+
53+
test 'discards cached socket after remote close' do
54+
c = Fluent::Plugin::ForwardOutput::SocketCache.new(10, $log)
55+
server = TCPServer.open('127.0.0.1', unused_port(protocol: :tcp))
56+
first_sock = TCPSocket.new('127.0.0.1', server.addr[1])
57+
first_peer = server.accept
58+
59+
c.checkout_or('key') { first_sock }
60+
c.checkin(first_sock)
61+
62+
first_peer.close
63+
waiting(5) do
64+
until IO.select([first_sock], nil, nil, 0)
65+
sleep 0.01
66+
end
67+
end
68+
69+
second_peer = nil
70+
second_sock = c.checkout_or('key') do
71+
sock = TCPSocket.new('127.0.0.1', server.addr[1])
72+
second_peer = server.accept
73+
sock
74+
end
75+
76+
assert_not_same(first_sock, second_sock)
77+
assert_true(first_sock.closed?)
78+
ensure
79+
c&.clear
80+
first_peer&.close rescue nil
81+
second_peer&.close rescue nil
82+
server&.close rescue nil
83+
end
5184
end
5285

5386
sub_test_case 'checkin' do

0 commit comments

Comments
 (0)