Skip to content

Fix two unclosed-resource leaks on error/retry paths in infer.py - #64

Open
nazarli-shabnam wants to merge 1 commit into
baidu:mainfrom
nazarli-shabnam:fix/close-leaked-resources-on-error-paths
Open

Fix two unclosed-resource leaks on error/retry paths in infer.py#64
nazarli-shabnam wants to merge 1 commit into
baidu:mainfrom
nazarli-shabnam:fix/close-leaked-resources-on-error-paths

Conversation

@nazarli-shabnam

Copy link
Copy Markdown

Summary

Two small, unrelated-to-any-open-work resource leaks in infer.py, both on error/retry paths (the code that's hardest to exercise in normal testing, so easiest to get wrong).

1. infer_one: streaming response leaked on 502 retry (infer.py, infer_one)

resp = requests.post(..., stream=True)
if resp.status_code == 502 and attempt < MAX_RETRIES - 1:
    time.sleep(3 * (attempt + 1))
    continue

The request is made with stream=True, so the underlying connection isn't released back to the pool until the body is read or resp.close() is called. On a 502, the loop jumps straight back to the top and issues a brand new requests.post(...) without ever closing the previous resp. MAX_RETRIES is 5, and run() fans this out across --concurrency (default 8) worker threads, so under a flaky/overloaded server — the exact condition a 502 signals — connections leak fastest right when they're scarcest.

Fix: call resp.close() before continue.

2. start_server: log file handle leaked when the SGLang subprocess exits early (infer.py, start_server)

log_file = open(args.server_log, "w", encoding="utf-8")
process = subprocess.Popen(cmd, env=env, stdout=log_file, stderr=subprocess.STDOUT)
process._log_file = log_file
...
if process.poll() is not None:
    log_file.flush()
    raise RuntimeError(f"SGLang server exited early. Check {args.server_log}")

The file handle's lifecycle is meant to be managed via stop_server(), which does process._log_file.close() — but stop_server() is only invoked from the other failure branch (the SERVER_TIMEOUT path, a few lines below). This early-exit RuntimeError path bypasses stop_server() entirely and never closes log_file.

Fix: call log_file.close() right after the existing flush(), before raising.

Why these and not something already open

I checked this against the repo's open issues/PRs before picking these (there's a lot of existing work on infer.py: issue #17 and PRs #21/#26/#34/#36/#13 already cover the PDF tmpdir leak, --image_mode validation in PDF mode, the avg_decode division skew, dataset sort order, and hardcoded ngram params). Neither of the two leaks fixed here is mentioned in any open issue or PR, and the fix for each is a single added line with no behavioral change beyond releasing the resource.

Changes

  • infer.py: resp.close() added before the 502-retry continue in infer_one.
  • infer.py: log_file.close() added before the RuntimeError in start_server.

Test plan

  • python -m py_compile infer.py — passes
  • Manually traced both modified branches to confirm control flow and messages are unchanged — only the resource-close call is new
  • Could not exercise a live 502 response or a crashing SGLang subprocess end-to-end (no SGLang server/GPU available in this environment); the change is a pure "close what's no longer needed" fix with no other behavioral impact, so risk is low, but a maintainer with a running SGLang setup may want to sanity-check the retry/crash paths directly.

- infer_one: close() the streaming response before retrying on a 502,
  instead of letting requests hold the connection open while a new
  request is issued. Under load (502 = server busy) this was leaking
  a TCP connection per retry, up to MAX_RETRIES times per job, across
  all concurrent workers — worst-case exactly when connections are
  already scarce.
- start_server: close log_file before raising RuntimeError when the
  SGLang subprocess exits early. The file handle was only closed in
  stop_server(), which this failure path never reaches, leaking the
  handle whenever the server crashes on startup.
@kushdab

kushdab commented Jul 11, 2026

Copy link
Copy Markdown

Both fixes are correct and match the actual infer.py code on main:

  • start_server: the early-exit RuntimeError branch is the only path into that function that doesn't go through stop_server() (the timeout path a few lines below calls stop_server(process), which does process._log_file.close()). Adding log_file.close() right after flush() closes that gap cleanly — no behavior change, matches the pattern stop_server already uses.
  • infer_one: stream=True on requests.post means the connection stays checked out of the pool until the body is read or resp.close() runs. The 502-retry branch jumped straight to a new requests.post(...) without releasing the old one, so this was a real leak that gets worse under exactly the load pattern (--concurrency, flaky server) that triggers 502s in the first place. resp.close() before continue is the right fix.

One related leak this PR doesn't cover, in the same function: the generic except Exception as e: block a few lines below (catching resp.raise_for_status() errors for non-502 statuses, or anything collect_stream_silent raises mid-stream) also retries/returns without ever closing resp. Since resp is only in scope inside the try, and the exception could occur either before or after resp is created, the safest fix is a finally that closes it if bound:

for attempt in range(MAX_RETRIES):
    resp = None
    try:
        resp = requests.post(
            f"{SERVER_URL}/v1/chat/completions",
            headers={"Content-Type": "application/json"},
            data=json.dumps(payload),
            timeout=REQUEST_TIMEOUT,
            stream=True,
        )
        if resp.status_code == 502 and attempt < MAX_RETRIES - 1:
            time.sleep(3 * (attempt + 1))
            continue
        resp.raise_for_status()
        result = collect_stream_silent(resp, output_file)
        print(f"  [{idx}] {name}: {result['tokens']} tokens, {result['decode_time']:.1f}s")
        return result
    except Exception as e:
        if attempt < MAX_RETRIES - 1:
            print(f"  [{idx}] {name}: retry {attempt + 1}/{MAX_RETRIES} ({e})")
            time.sleep(3 * (attempt + 1))
            continue
        print(f"  [{idx}] {name}: FAILED ({e})")
        return {"tokens": 0, "decode_time": 0, "text": ""}
    finally:
        if resp is not None:
            resp.close()

Closing an already-fully-consumed stream response is a safe no-op, so an unconditional close in finally covers the success path too without changing its behavior. Worth folding in since it's the same underlying pattern this PR is already fixing, just reached from a different exit out of the same loop.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants