Client: cpp
macOS rejects an RLIMIT_AS limit with EINVAL, so the child always exited with 2
and the case failed there. The child now exits with 77 in that case and the
case reports that it has nothing to check; any other setrlimit failure still
fails it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Client: cpp
TConnection::transition() can close a connection while a request is processed
inline, which returns the connection object to the server to be reused or
deleted; workSocket() then read the connection's members. transition() now
reports whether the connection is still owned by the calling I/O thread, and
workSocket() stops working the socket as soon as it is not.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Client: cpp
A failed accept() reached libevent's listen callback as an exception and
ended the only listener loop. The callback now logs that one failure and
keeps accepting, the same boundary the connection and notification
callbacks already use.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Client: cpp
TServerSocket::listen() and TNonblockingServerSocket::listen() resolved the
bind address with AI_PASSIVE|AI_V4MAPPED, while TSocket::open() resolves the
connect address with AI_PASSIVE|AI_ADDRCONFIG. AI_ADDRCONFIG does not count a
loopback address as a configured one, so on a host that carries ::1 on lo and
no other IPv6 address -- a default Docker container, for instance -- the two
disagree about what "localhost" means: the server binds ::1 and the client
dials 127.0.0.1, and the connect fails with ECONNREFUSED. The IPV6_V6ONLY=0
the server sets does not help, because the bind is to ::1 rather than to ::.
Last worked in 0.13.0. THRIFT-5186 removed AI_ADDRCONFIG from the server
sockets in 9b9567b23 so that a host with no configured address could still
resolve localhost, and gave TSocket::open() a retry without the flag for the
same case. The client kept the flag; the servers did not, and the asymmetry
is what breaks. THRIFT-5880 later widened that client retry to EAI_ADDRFAMILY,
but it is conditional on resolution failing and does not fire here.
Resolve on the server with AI_ADDRCONFIG as well, and fall back to resolving
without it when that leaves nothing to bind -- the same shape TSocket::open()
already has, so both sides agree again while THRIFT-5186's case keeps working.
The fallback is load-bearing rather than defensive: an explicit "::1" fails
with EAI_ADDRFAMILY under AI_ADDRCONFIG on such a host. The ANDROID branch is
now redundant and goes away; Android already passed AI_ADDRCONFIG and only
gains the fallback, since AI_V4MAPPED is ignored for AF_UNSPEC queries.
lib/cpp/test/TServerSocketTest.cpp's test_bind_to_address already covered this
for TServerSocket and failed in such a container; TNonblockingServerSocket had
no equivalent coverage and now has one. Both were confirmed to fail against the
unmodified library before the fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Client: cpp
bad_alloc_does_not_end_the_process failed intermittently on AppVeyor, 11 of
229 jobs since it was added, either crashing or hanging until the 300 s
ctest timeout. Two independent races, one behind each symptom, so both are
fixed here.
The crash: the fixture stopped the server and joined the serve thread but
never stopped the ThreadManager. Members are destroyed in reverse
declaration order, so the server went before the thread manager, and
~TNonblockingServer deletes every TConnection and the IO threads along with
their notification pipe. A pool task still unwinding from the injected
failure then reached notifyIOThread() -- and close() -- on the TConnection
it holds by raw pointer. The fixture destructor now stops the ThreadManager
after joining the serve thread and before the members go;
ThreadManager::stop() returns only once each worker has finished the task in
its hands, so nothing is left running over the objects being destroyed.
The hang: FailsFirstCallProcessor failed whichever call reached process()
first, tracked in a plain bool that both pool workers read and wrote. When
the call from canCommunicate() won that race it consumed the failure and got
no reply, and its client had no receive timeout, so the test body blocked
until ctest killed it. The flag is now claimed with an atomic exchange, the
processor signals a Monitor as it claims it, and the test waits for that
signal before opening the second connection -- so the failure is always
taken by the call meant to receive it. The exchange matters on its own:
read-then-write let two calls each see the flag unset and both throw, which
is what the Windows logs showed as a doubled bad_alloc line.
canCommunicate() also sets a 10 s receive timeout. Every call it makes is a
localhost round-trip of a few bytes, so this only bounds the failure: a
regression reports in seconds with the transport exception rather than as an
opaque 300 s timeout.
Verified by forcing each schedule rather than waiting for the race:
- delaying the first arrival at process() so the second connection claims
the failure reproduces the hang exactly; without the wait the case fails
in 10 s with THRIFT_EAGAIN, with it the case passes.
- holding the failing task in flight past the end of the test body puts
ASan on the use-after-free with the ThreadManager stop removed --
notifyIOThread() reading a TConnection freed by ~TNonblockingServer()
through Fixture::~Fixture() -- and reports nothing with it present.
Then 5 clean runs of the full suite, and 200 runs of this case under ASan
with eight in parallel.
Not touched: allocation_failure_on_the_io_thread_does_not_end_the_process
fails under ASan because its RLIMIT_AS child collides with ASan's own
allocator. That reproduces identically on master.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Client: cpp
TConnection reserved the whole declared frame the moment it read the
four-byte length prefix: transition() reallocated the read buffer to the
frame size before any payload byte had arrived, so a peer could make the
server reserve up to the maximum frame size by sending only a header.
The buffer now grows toward the frame size in SOCKET_RECV as bytes are
actually read -- by doubling, with the growth guarded against uint32_t
overflow -- so the reservation tracks the payload received. transition()
reserves only the length prefix and a small initial read. A frame that
fully arrives ends at the same buffer size as before; a header with no
payload reserves next to nothing. A large frame that does arrive pays
the amortized doubling copy any grow-as-you-go buffer pays.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Client: cpp
TNonblockingServer kept its own frame-size limit and created accepted
sockets with no TConfiguration, so an operator who configured a maximum
frame or message size got no effect: the server enforced only its own
frame-size ceiling, and each accepted socket used the default 100 MB
message budget.
The server now holds a TConfiguration. getMaxFrameSize()/setMaxFrameSize()
operate on it, and it is handed to every accepted socket through the
server transport, so the configured maximum message size bounds each
connection and the layered transports built on top of it. A frame larger
than the message budget is refused before the read buffer is grown for
it, alongside the existing frame-size check.
setMaxFrameSize() clamps to INT_MAX, since the configuration stores the
frame size as int; a value above INT_MAX would otherwise wrap to a
negative int and read back as a huge size_t, disabling the limit.
TTransport::setConfiguration() now re-seeds the message-size budget from
the new configuration. It previously swapped the pointer without
re-seeding, so a configuration installed after construction never took
effect; it has no callers within the library.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Client: cpp
libevent is a C library, and TNonblockingServer hands it two callbacks:
eventHandler(), which drives TConnection::workSocket(), and notifyHandler(),
which drives TConnection::transition(). An exception thrown under either does
not reach a handler -- it unwinds into C frames and reaches std::terminate.
transition() can throw: growing the read buffer to the frame size the peer
declared ends in throw std::bad_alloc() when the allocation fails, and that is
the only throw in the function that is not already caught. The frame size is the
peer's choice up to the server's maximum, so the peer chooses the size of the
allocation that has to succeed.
Measured against the unmodified library, with the address space capped and the
maximum frame size raised above it: one connection sending a four byte frame
header and nothing else brings down the process with "terminate called after
throwing an instance of 'std::bad_alloc'" and SIGABRT. That is worse than the
exit(1) removed in the previous commit -- it needs no thread manager, so it
applies to the default configuration.
Both callbacks now catch at the boundary and close the connection, which is
what every other error path in workSocket() and transition() already does. The
process keeps serving its other connections; measured again, it does.
The test forks, caps the address space in the child and lets the child exit 0
if it survives, because the cap cannot be undone for the rest of the suite. It
is guarded for POSIX; RLIMIT_AS has no Windows equivalent.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Client: cpp
Task::run() caught std::bad_alloc apart from every other exception and answered
it with exit(1), so one request whose processing could not be allocated ended
the process for every other client connected to it. The catch clause below it
already handles std::exception -- logs it and hands the connection back to the
I/O thread -- and the inline, non-thread-pooled path in the same file has always
just logged and closed the connection. The special case is removed and
std::bad_alloc takes that same route.
Measured: a TNonblockingServer with a thread manager, one framed call whose
processing throws std::bad_alloc, and the process is gone with status 1. The
allocation that fails need not be in application code -- generated container
deserialization allocates from a declared element count before any handler
runs, which is what makes this reachable from the wire.
lib/cpp/test/TNonblockingServerTest.cpp gains a case; the fixture learns to run
a server on the thread-pooled path, since every existing case uses the inline
one. Against the unmodified library the test binary does not fail an assertion,
it exits mid-run and takes the other four cases with it.
The case deliberately does not wait for a reply on the doomed connection. A
task that throws writes nothing, and the connection reads an empty write buffer
as no reply being owed. That is not the same thing: a void method is not a
oneway method, it still owes a reply, and an exception is one of the replies it
can owe. So that one caller waits for ever. The confusion is pre-existing,
applies to every exception this catch handles, and differs from the inline path,
which closes the connection; left alone here and recorded as its own question.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TNonblockingServer caps the frame it will accept at its own MAX_FRAME_SIZE,
256 * 1024 * 1024 since THRIFT-1337 landed it in 2011 -- years before
TConfiguration existed. The library-wide default is
TConfiguration::DEFAULT_MAX_FRAME_SIZE, 16384000, described in its own header
as "used consistently across all Thrift libraries". The two disagree by a
factor of 16:
TNonblockingServer::getMaxFrameSize() 268435456
TConfiguration::DEFAULT_MAX_FRAME_SIZE 16384000
TConfiguration appears nowhere in TNonblockingServer.h or .cpp, so the server
neither reads a configured value nor inherits the default; it starts from its
own constant and only setMaxFrameSize() moves it. That constant is what sizes
the read buffer, since TConnection::transition() resets the buffer to the
length the peer declared before any payload byte has arrived.
Java's AbstractNonblockingServer has no such gap: its per-frame limit comes
from trans_.getMaxFrameSize(), that is from TConfiguration, and it keeps a
separate aggregate read-buffer budget on top of it.
So point MAX_FRAME_SIZE at TConfiguration::DEFAULT_MAX_FRAME_SIZE. The
constant is private, so this is not an API change, and setMaxFrameSize()
still overrides it.
This lowers a shipped default. A deployment that accepts frames between
16,384,000 and 268,435,456 bytes on TNonblockingServer today and does not
call setMaxFrameSize() will start closing those connections, so it needs a
release note.
One test. It fails before the change on 268435456 != 16384000, and pins that
the setter still wins so the escape hatch cannot quietly go away.
Client: cpp
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* use override
* use make_shared
* use emplace
* use range for
* fix error on MSVC
* replace boost functions with std functions
* fix static analyzer warnings
* check api return value
* initialize member
* check the return value of SSL_peek > 0
* add override
* make use of C++11 override keyword
* added const specifier to TTransport::getOrigin()
* added more const correctness to the compiler
* make use of auto keyword
* replaced usage of NULL with nullptr
* make use of explicitly-defaulted function definition
* extended changelog
Following changes are made to fix the build
* Add USE_..._THREAD compiler definitions correctly
* Link to boost_thread and boost_system when configured with boost thread
* Link to pthread if platform is posix and std thread is used
* Use PlatformThreadFactory in test code
