Jens Geyer created THRIFT-6210:
----------------------------------
Summary: Go: an unrecovered panic in a handler ends the whole
server process
Key: THRIFT-6210
URL: https://issues.apache.org/jira/browse/THRIFT-6210
Project: Thrift
Issue Type: Improvement
Components: Go - Library
Reporter: Jens Geyer
h2. What happens
{{lib/go/thrift}} never calls {{recover()}} — there is no call anywhere in the
package outside {{configuration_test.go}}. {{TSimpleServer}} runs each accepted
connection on a goroutine of its own ({{simple_server.go:211}}) and reaches
{{processor.Process}} from there with nothing in between:
{code}
go func() {
defer p.wg.Done()
defer cancel()
defer client.Close()
if err := p.processRequests(client); err != nil {
...
}
}()
{code}
Go ends the whole process on an unrecovered panic in any goroutine, so a panic
anywhere inside a handler takes the server down — every other connection with
it, not only the one being served. Reproduced against master @ {{5c5e93e1e}}
with a hand-written processor whose handler panics: one request, and the
process exits with status 2.
{code}
panic: handler blew up
goroutine 50 [running]:
main.boomFunc.Process(...)
main.(*proc).Process(...)
github.com/apache/thrift/lib/go/thrift.(*TSimpleServer).processRequests(...)
lib/go/thrift/simple_server.go:368
github.com/apache/thrift/lib/go/thrift.(*TSimpleServer).innerAccept.func1()
lib/go/thrift/simple_server.go:215
exit status 2
{code}
h2. The panic has to come from the handler, not from the wire
The peer-facing read path was swept for inputs that panic rather than return an
error, using native Go fuzzing over the library on master @ {{5c5e93e1e}}:
twelve targets covering TBinaryProtocol (strict and non-strict),
TCompactProtocol, TJSONProtocol, TSimpleJSONProtocol, THeaderProtocol including
its transform-ID and info-header parsing, TFramedTransport, a full server-side
dispatch hop through TMultiplexedProcessor, and {{ParseTuuid}}. Enveloped and
bare-struct entry points both. About 28 million executions in total (120s per
target, coverage-guided), no panic in any target.
That matches what the code says: generated Go read code returns errors, and
wire-supplied container counts and string lengths are bounds-checked before
they size an allocation. So the panic has to originate in application code — a
nil map write, a slice index, a failed type assertion, or any library the
handler calls.
That makes this a question about the library's default behaviour, not a defect
reachable from outside.
h2. The same panic is survivable over HTTP
{{NewThriftHandlerFunc}} runs under {{net/http}}, which recovers per request,
logs the stack and drops that one connection. So a handler bug that is merely
an error for a Thrift-over-HTTP server is fatal for the socket server. The
inconsistency is worth deciding about one way or the other.
h2. What an application can already do
No library change is needed to fix this in an application. Processor middleware
covers it in about twenty lines, and keeps the connection by reporting a
{{TApplicationException}} the way generated code does:
{code}
func recoverMiddleware(name string, next thrift.TProcessorFunction)
thrift.TProcessorFunction {
return thrift.WrappedTProcessorFunction{
Wrapped: func(ctx context.Context, seqID int32, in, out
thrift.TProtocol) (ok bool, err thrift.TException) {
defer func() {
if r := recover(); r != nil {
x := thrift.NewTApplicationException(
thrift.INTERNAL_ERROR,
fmt.Sprintf("panic in %s",
name),
)
out.WriteMessageBegin(ctx, name,
thrift.EXCEPTION, seqID)
x.Write(ctx, out)
out.WriteMessageEnd(ctx)
out.Flush(ctx)
ok, err = true, nil
}
}()
return next.Process(ctx, seqID, in, out)
},
}
}
processor = thrift.WrapProcessor(processor, recoverMiddleware)
{code}
Verified: with the middleware the server survives, the client receives the
exception, and the process stays up.
h2. The question
Should the library do this itself?
For: {{net/http}} does it, the blast radius is the whole process rather than
one request, and one request ending one request is the behaviour every other
Thrift binding already has — an uncaught exception on a per-connection thread
ends that thread, not the runtime.
Against: a substantial part of the Go community holds that libraries should not
swallow panics, on the grounds that program state after a panic is unknown and
continuing can be worse than stopping. A server that keeps running with a
corrupted invariant is a real cost, not a hypothetical one.
Options, roughly in order of how much they commit to:
# Leave the behaviour alone; document it, and ship the middleware above in the
library as an opt-in (for example {{thrift.RecoverMiddleware}}), so
applications do not each write their own.
# Recover per request inside {{TSimpleServer}} by default, with a knob to turn
it off.
# Do nothing.
Option 1 looks like the smallest thing that removes the trap without deciding
the argument for everybody, but this needs a maintainer call rather than a
patch.
Related: THRIFT-5073 raised "we can log panic situation and return
TApplicationException to client instead of EOF" as one motivation for processor
interceptors. Middleware landed since, but no recovering middleware ships with
the library.
Investigation and this description were AI-assisted (Claude Opus 5); the
findings were verified by execution against master.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
