Skip to content

Eliminate GC refcounting cycle on _connect exception - #4195

Open
rschlaikjer wants to merge 4 commits into
redis:masterfrom
rschlaikjer:rs-connect-refcount-cycle
Open

Eliminate GC refcounting cycle on _connect exception#4195
rschlaikjer wants to merge 4 commits into
redis:masterfrom
rschlaikjer:rs-connect-refcount-cycle

Conversation

@rschlaikjer

@rschlaikjer rschlaikjer commented Jul 17, 2026

Copy link
Copy Markdown

Description of change

Currently, in _connect, if an exception is thrown while trying different address infos, it is cached to be thrown only if no connection succeeds. However, storing a caught exception inside the local stack frame creates a circular reference: the exception's traceback contains a pointer to the current frame, which contains the err local, which points to the exception again. This creates garbage that can only be collected by the fallback mark and sweep python GC.

Break this cycle by clearing the local exception value on re-raise, allowing the error to be reclaimed via refcounting. This is useful for applications that disable or otherwise limit the python garbage collector.

Pull Request check-list

Please make sure to review and check all of these items:

  • Do tests and lints pass with this change?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Is there an example added to the examples folder (if applicable)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.


Note

Low Risk
Localized TCP connect error-handling cleanup with no behavior change beyond memory/GC semantics; covered by new unit tests.

Overview
Fixes a reference cycle in TCP Connection._connect when iterating getaddrinfo results: a failed attempt stores the caught OSError in err, and re-raising it leaves the traceback pointing at the frame that still holds err, which only cyclic GC can collect.

On successful connect after an earlier failure, err is set to None before returning the socket. When all addresses fail, the final re-raise uses try/finally to clear err after propagation so refcounting can drop the exception immediately—relevant for apps that limit or disable the cyclic garbage collector.

Adds tests that inspect the _connect stack frame and assert err is cleared on total failure and when a later address succeeds.

Reviewed by Cursor Bugbot for commit 9a152dd. Bugbot is set up for automated code reviews on this repo. Configure here.

@sean-kim05 sean-kim05 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This matches the idiom CPython uses in socket.create_connection itself (try: raise err finally: err = None), so it's well-precedented. 👍

One thing worth surfacing for reviewers: the added err = None just before return sock isn't redundant with the finally. It covers the case where an earlier address from getaddrinfo fails (populating err) and a later one succeeds — without it, the returning frame still holds that earlier exception, whose traceback references the frame, keeping the cycle alive until the GC runs. So both clears pull their weight; might be worth a one-line comment on that second one since its purpose is less obvious than the finally.

@petyaslavova

Copy link
Copy Markdown
Collaborator

Hey @rschlaikjer, thank you for your contribution! I'll review it shortly.

eeshsaxena

This comment was marked as low quality.

@Sanjays2402

Copy link
Copy Markdown
Contributor

same pattern is live in NodesManager.initialize: exception = e (cluster.py:2706) stays bound in that frame and then gets attached as __cause__ on the RedisClusterException raised at 2775, so you get the identical exception -> traceback -> frame -> local loop, on the connect path too. worth clearing it there in the same PR, or do you want to keep this one narrow?

@petyaslavova petyaslavova left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @rschlaikjer, the change looks good!
Before merging, can you please also cover the case where the first sock.connect has failed, but the next one succeeds? It will be good to have this other flow covered as well...

@petyaslavova petyaslavova added maintenance Maintenance (CI, Releases, etc) waiting-for-response labels Aug 3, 2026
@rschlaikjer

Copy link
Copy Markdown
Author

@petyaslavova, thanks for the review. Added a test for the failure followed by success case.

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

Labels

maintenance Maintenance (CI, Releases, etc) waiting-for-response

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants