GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server

[raulcd]

Rationale for this change

PyFlightServer keeps a reference towards the Python server via OwnedRefNoGIL server_, the Python server also keeps a reference of the C++ PyFlightServer creating a cycle that is never freed during the process lifetime.

What changes are included in this PR?

Create a new ReleasePythonServerRef method that is called after any server.Shutdown (including at __exit__) allowing for the OwnedRefNoGIL to be cleared breaking the cycle. This lets normal reference counting free the previously leaked Python object.

Are these changes tested?

Yes, the newly added tests were failing leaking the references before the fix.
Currently a test demonstrating a leak when not calling server.Shutdown is added for discussion purposes.

Are there any user-facing changes?

No

• GitHub Issue: [Python][FlightRPC] FlightServerBase is never freed, leaking the server and everything it references #50684

[raulcd]

@github-actions crossbow submit wheel-manylinux-2-28-cp313-cp313-amd64

[github-actions]

Revision: 6c7c46a

Submitted crossbow builds: ursacomputing/crossbow @ actions-e80f3b2c61

Task	Status
wheel-manylinux-2-28-cp313-cp313-amd64

[pitrou]

Create a new ReleasePythonServerRef method that is called after any server.Shutdown (including at __exit__) allowing for the OwnedRefNoGIL to be cleared breaking the cycle. This lets normal reference counting free the previously leaked Python object.

This won't work is neither shutdown nor __exit__ is called, right?

[raulcd]

This won't work is neither shutdown nor exit is called, right?

Correct, this is why the newly added test which doesn't call shutdown or exit has an xfail mark.

[raulcd]

@lidavidm I'd appreciate some feedback on the approach here as you probably have a better mental model of how all this fits together.

[lidavidm]

It seems reasonable to me. It's been a while since I looked at this code. I do agree with Antoine that maybe we just make C++ hold a weakref to the Python side, presumably nobody is intentionally leaking a server and assuming it will keep running...?

[pitrou]

Here's an idea: instead of:

    def shutdown(self):
        with nogil:
            check_flight_status(self.server.get().Shutdown())
        self.server.get().ReleasePythonServerRef()

How about we do:

    def serve(self):
        self.server.get().SetPythonServerRef(self)
        try:
            with nogil:
                check_flight_status(self.server.get().ServeWithSignals())
        finally:
            self.server.get().ReleasePythonServerRef()

and avoid passing self in the PyFlightServer constructor?

[raulcd]

How about we do:

   def serve(self):
       self.server.get().SetPythonServerRef(self)
       try:
           with nogil:
               check_flight_status(self.server.get().ServeWithSignals())
       finally:
           self.server.get().ReleasePythonServerRef()

I don't think calling serve is mandatory. We have plenty of tests where we call the constructor and dispatch without ever calling serve and those still call the individual methods (DoGet/etc) which require the reference. The reference should exist from the Init() call, in the constructor.

[pitrou]

We have plenty of tests where we call the constructor and dispatch without ever calling serve and those still call the individual methods (DoGet/etc) which require the reference.

Hmm, I see. I would consider these tests a bit artificial (end users aren't supposed to call these methods directly AFAIU), but that's a good argument for keeping the current behavior I suppose.

[raulcd]

I agree that the tests are artificial but our current documentation states that calling serve is not necessary:

arrow/python/pyarrow/_flight.pyx

Lines 2893 to 2900 in 3deec0b

	cdef class FlightServerBase(_Weakrefable):
	"""A Flight service definition.
	To start the server, create an instance of this class with an
	appropriate location. The server will be running as soon as the
	instance is created; it is not required to call :meth:`serve`.
	Override methods to define your Flight service.

[pitrou]

The server will be running as soon as the instance is created; it is not required to call :meth:serve.

I had missed that part. It's a bit unfortunate :-/

[raulcd]

moving into review to correctly reflect state as we have already been reviewing it :)

[Copilot]

Pull request overview

This PR addresses GH-50684 by breaking a reference cycle between the C++ PyFlightServer (which owns an OwnedRefNoGIL to the Python FlightServerBase) and the Python object (which owns the C++ server), allowing servers to be freed after shutdown instead of leaking for the lifetime of the process.

Changes:

• Add PyFlightServer::ReleasePythonServerRef() to clear the C++ → Python reference and break the cycle.
• Invoke ReleasePythonServerRef() after shutdown paths in the Cython bindings (explicit shutdown() and the server finalizer).
• Add Python tests asserting the server object is freed after shutdown / context-manager exit, plus an xfail test documenting the “no shutdown” leak case.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
python/pyarrow/tests/test_flight.py	Adds regression tests validating server objects are freed after shutdown/exit (and documents current behavior without shutdown).
python/pyarrow/src/arrow/python/flight.h	Declares ReleasePythonServerRef() on PyFlightServer.
python/pyarrow/src/arrow/python/flight.cc	Implements ReleasePythonServerRef() by resetting the stored Python reference.
python/pyarrow/includes/libarrow_flight.pxd	Exposes ReleasePythonServerRef() to Cython.
python/pyarrow/_flight.pyx	Calls ReleasePythonServerRef() after shutdown and in the server finalizer to break the cycle.

[pitrou]

By the way, is the Wait call useful here? My understanding is that Wait can be used from another thread than the thread that calls Shutdown, but calling it synchronously after Shutdown should return immediately? @lidavidm

[lidavidm]

Indeed it should...so this seems redundant.

[raulcd]

Should I try removing it here? Probably not worth just creating a follow up issue for it.

[pitrou]

Yes, I think you can try removing it.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

python/pyarrow/src/arrow/python/flight.cc:93

• ReleasePythonServerRef() calls server_.reset(), which ultimately does Py_XDECREF and therefore requires the Python runtime to be initialized and the GIL to be held. Since this method is exposed to Cython as nogil (and is also called from shutdown/finalizers), it should defensively acquire the GIL (and avoid touching refcounts if the interpreter is already finalized) to prevent potential crashes if it is ever invoked without the GIL or during interpreter teardown.

void PyFlightServer::ReleasePythonServerRef() {
  // Resets OwnedRefNoGIL to break the reference cycle between the C++ FlightServerBase
  // and the Python object.
  server_.reset();
}

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

python/pyarrow/src/arrow/python/flight.cc:93

• ReleasePythonServerRef() calls OwnedRefNoGIL::reset(), but reset() does not acquire the GIL (only the destructor does). Since this method is declared in a nogil Cython extern block, it must be safe to call without the GIL; as written it can decref a Python object without the GIL, which is unsafe and can crash.

void PyFlightServer::ReleasePythonServerRef() {
  // Resets OwnedRefNoGIL to break the reference cycle between the C++ FlightServerBase
  // and the Python object.
  server_.reset();
}

python/pyarrow/tests/test_flight.py:1157

• This xfail is non-strict, so if the underlying leak is fixed later the test will become an XPASS and may not fail CI, leaving a stale xfail marker in place.

@pytest.mark.xfail(
    reason="GH-50684: FlightServerBase is not freed on delete without shutdown"
)

[raulcd]

@github-actions crossbow submit -g python

[github-actions]

Revision: 5b600c2

Submitted crossbow builds: ursacomputing/crossbow @ actions-ed9acf3136

Task	Status
example-python-minimal-build-fedora-conda
example-python-minimal-build-ubuntu-venv
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hdfs-2.9.2
test-conda-python-3.11-hdfs-3.2.1
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-2.0.3-numpy-1.23.2
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.12-pandas-latest-numpy-latest
test-conda-python-3.13
test-conda-python-3.13-pandas-latest-numpy-1.26.2
test-conda-python-3.13-pandas-latest-numpy-latest
test-conda-python-3.14
test-conda-python-3.14-cpython-debug
test-conda-python-3.14-pandas-nightly-numpy-nightly
test-conda-python-3.14-pandas-upstream_devel-numpy-nightly
test-conda-python-emscripten
test-debian-13-python-3-amd64
test-debian-13-python-3-i386
test-fedora-42-python-3
test-ubuntu-24.04-python-3