Skip to content

HttpSys: a context reads its engine from a request/response already recycled or freed (use after free); the worker thread then dies silently #278

Open

Description

@Stemonik

Where

TDextHttpSysRequest._Release and TDextHttpSysResponse._Release (v1.0.1, lines 834 and 1113):

LContext := FContext;
if Assigned(FEngine) then
  FEngine.RecycleRequest(Self)      // back to the pool, or Free when the pool is full
else
  Destroy;
if LContext <> nil then
  LContext.ReleaseRequestReference; // <- reads the engine through FRequest

and TDextHttpSysContext.ReleaseRequestReference / ReleaseResponseReference (lines 613-623):

if (TInterlocked.Decrement(FRefCount) = 0) and not FDestroying then
  TDextHttpSysEngine(FRequest.FEngine).ReleaseContext(Self);

When the last reference of the context goes away, FRequest (or FResponse) has already been given back:

  • if its pool was full, Release set FEngine := nil and freed it: the context reads freed memory (nil, or whatever the memory manager put there since);
  • if it went back to the pool, another thread may already have taken it and called Init on it.

The shared request and response pools hold 64 objects each, so under load (200 connections) the "pool full, free it" path is common: we counted tens of thousands of requests and responses created during a 15 s run.

What happens

The access violation is raised inside TDextHttpSysWorker.Execute, which has no try/except around the loop: the worker thread ends silently. With 32 workers the server keeps answering for a while, slower and slower, until no worker is left and it stops answering at all.

How to see it

With the default settings it is rare (we did not see it in our normal runs). It becomes frequent when fewer contexts are kept per thread (we were measuring the memory of the engine with a smaller per-thread pool, a constant 64 in ReleaseContext today): with 1 context per thread the worker threads went from 45 to 13 and the server stopped answering after about 1,000 requests; a counter of the exceptions caught around the loop showed the access violations in ReleaseContext.

Reproduced again on main with only the memory changes I am sending in a separate PR on top (they make the contexts per thread an option) and HttpSysContextsPerThread = 1: 50 rows at 200 connections, 10 s, two rounds. The first round passed; in the second, 200 errors, then the batch insert got 0 good answers out of 19,801 and the server used no more CPU: no worker left. With the fix, 0 errors in both rounds.

Fix (PR follows)

The context keeps its own reference to the engine, set when the engine creates it, and ReleaseRequestReference / ReleaseResponseReference use it instead of going through FRequest / FResponse. With the fix: no exceptions and no stop with 1 context per thread, and 0 errors in an A/B/B/A run of all our endpoints at 50 and 200 connections.

Separately, I would suggest a try/except around the body of the worker loop that logs the exception and keeps the thread alive: an unexpected exception in one request should not take a worker away for good. I left it out of the PR because the logging is your choice.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions