You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "raulcd (via GitHub)" <gi...@apache.org> on 2026/07/28 14:35:46 UTC
[PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
raulcd opened a new pull request, #50687:
URL: https://github.com/apache/arrow/pull/50687
### 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 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #50687:
URL: https://github.com/apache/arrow/pull/50687#issuecomment-5117964650
>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 :-/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3667664126
##########
python/pyarrow/_flight.pyx:
##########
@@ -3234,6 +3235,7 @@ cdef class FlightServerBase(_Weakrefable):
raise ValueError("shutdown() on uninitialized FlightServerBase")
with nogil:
check_flight_status(self.server.get().Shutdown())
+ self.server.get().ReleasePythonServerRef()
Review Comment:
Is this one actually necessary? And if so, should we use a try/finally block?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #50687:
URL: https://github.com/apache/arrow/pull/50687#issuecomment-5105748198
@github-actions crossbow submit wheel-manylinux-2-28-cp313-cp313-amd64
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3674428888
##########
python/pyarrow/src/arrow/python/flight.cc:
##########
@@ -86,6 +86,12 @@ PyFlightServer::PyFlightServer(PyObject* server, const PyFlightServerVtable& vta
server_.reset(server);
}
+void PyFlightServer::ReleasePythonServerRef() {
+ // Resets OwnedRefNoGIL to break the reference cycle between the C++ FlightServerBase
+ // and the Python object.
+ server_.reset();
+}
Review Comment:
ReleasePythonServerRef() calls OwnedRefNoGIL::reset(), which ultimately does Py_XDECREF without acquiring the GIL (see OwnedRef::reset in arrow/python/common.h). Since this method is declared in a `nogil` Cython extern block, it is easy to call it while the GIL is not held, leading to undefined behavior / crashes. Make ReleasePythonServerRef acquire the GIL when needed (and avoid DECREF when the interpreter is finalized).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3673893414
##########
python/pyarrow/tests/test_flight.py:
##########
@@ -1128,6 +1130,43 @@ def test_flight_server_location_argument():
assert isinstance(server, FlightServerBase)
+# The following tests are for GH-50684, which was a memory leak
+# in FlightServerBase.
+def test_flight_server_is_freed():
+ # Calling server.shutdown manually should free the server object.
+ server = FlightServerBase('grpc://localhost:0')
+ server.shutdown()
+ server.wait()
+ ref = weakref.ref(server)
+ del server
+ gc.collect()
+ assert ref() is None
+
+
+def test_flight_server_is_freed_on_exit():
+ # Using FlightServerBase as a context manager should free
+ # the server object on exit.
+ with FlightServerBase('grpc://localhost:0') as server:
+ ref = weakref.ref(server)
+ del server
+ gc.collect()
+ assert ref() is None
+
+
+@pytest.mark.xfail(
+ reason="GH-50684: FlightServerBase is not freed on delete without shutdown"
+)
+def test_flight_server_is_freed_without_shutdown():
+ # GH-50684: Not calling server.shutdown() currently leaks the server object.
+ # This test is expected to fail until the issue is fixed but is included
+ # for completeness and further discussion.
Review Comment:
Ok, thanks for attempting this. Then it seems the weakref idea is dead in the water (we could do something more sophisticated where the finalizer calls Shutdown from a dedicated thread, but that's starting to get very complicated IMHO).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3675272431
##########
python/pyarrow/_flight.pyx:
##########
@@ -2887,6 +2887,7 @@ cdef class _FlightServerFinalizer(_Weakrefable):
status = server.Wait()
Review Comment:
Indeed it should...so this seems redundant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3667983955
##########
python/pyarrow/tests/test_flight.py:
##########
@@ -1128,6 +1130,43 @@ def test_flight_server_location_argument():
assert isinstance(server, FlightServerBase)
+# The following tests are for GH-50684, which was a memory leak
+# in FlightServerBase.
+def test_flight_server_is_freed():
+ # Calling server.shutdown manually should free the server object.
+ server = FlightServerBase('grpc://localhost:0')
+ server.shutdown()
+ server.wait()
+ ref = weakref.ref(server)
+ del server
+ gc.collect()
+ assert ref() is None
+
+
+def test_flight_server_is_freed_on_exit():
+ # Using FlightServerBase as a context manager should free
+ # the server object on exit.
+ with FlightServerBase('grpc://localhost:0') as server:
+ ref = weakref.ref(server)
+ del server
+ gc.collect()
+ assert ref() is None
+
+
+@pytest.mark.xfail(
+ reason="GH-50684: FlightServerBase is not freed on delete without shutdown"
+)
+def test_flight_server_is_freed_without_shutdown():
+ # GH-50684: Not calling server.shutdown() currently leaks the server object.
+ # This test is expected to fail until the issue is fixed but is included
+ # for completeness and further discussion.
Review Comment:
Ok, if we want to fix this, the solution is to make `PyFlightServer::_server` a [weakref](https://docs.python.org/3/c-api/weakref.html).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd merged PR #50687:
URL: https://github.com/apache/arrow/pull/50687
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3681174035
##########
python/pyarrow/_flight.pyx:
##########
@@ -2887,6 +2887,7 @@ cdef class _FlightServerFinalizer(_Weakrefable):
status = server.Wait()
Review Comment:
Yes, I think you can try removing it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #50687:
URL: https://github.com/apache/arrow/pull/50687#issuecomment-5111345495
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...?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #50687:
URL: https://github.com/apache/arrow/pull/50687#issuecomment-5117932759
I agree that the tests are artificial but our current documentation states that calling `serve` is not necessary:
https://github.com/apache/arrow/blob/3deec0b119a3fc009e86e473242a6b647eea94e8/python/pyarrow/_flight.pyx#L2893-L2900
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3668057264
##########
python/pyarrow/_flight.pyx:
##########
@@ -3234,6 +3235,7 @@ cdef class FlightServerBase(_Weakrefable):
raise ValueError("shutdown() on uninitialized FlightServerBase")
with nogil:
check_flight_status(self.server.get().Shutdown())
+ self.server.get().ReleasePythonServerRef()
Review Comment:
Commenting this line, the single failing test on `test_flight.py`:
```python
====================================================================================================== FAILURES ======================================================================================================
____________________________________________________________________________________________ test_flight_server_is_freed _____________________________________________________________________________________________
def test_flight_server_is_freed():
# Calling server.shutdown manually should free the server object.
server = FlightServerBase('grpc://localhost:0')
server.shutdown()
server.wait()
ref = weakref.ref(server)
del server
gc.collect()
> assert ref() is None
E AssertionError: assert <pyarrow._flight.FlightServerBase object at 0x7fbb4dba7380> is None
E + where <pyarrow._flight.FlightServerBase object at 0x7fbb4dba7380> = <weakref at 0x7fbb4dba7650; to 'pyarrow._flight.FlightServerBase' at 0x7fbb4dba7380>()
pyarrow/tests/test_flight.py:1143: AssertionError
============================================================================================== short test summary info ===============================================================================================
FAILED pyarrow/tests/test_flight.py::test_flight_server_is_freed - AssertionError: assert <pyarrow._flight.FlightServerBase object at 0x7fbb4dba7380> is None
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3667939037
##########
python/pyarrow/_flight.pyx:
##########
@@ -3234,6 +3235,7 @@ cdef class FlightServerBase(_Weakrefable):
raise ValueError("shutdown() on uninitialized FlightServerBase")
with nogil:
check_flight_status(self.server.get().Shutdown())
+ self.server.get().ReleasePythonServerRef()
Review Comment:
Without this one `test_flight_server_is_freed` fails. As per the release of the `shared_ptr` I haven't investigated that line, I can take a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #50687:
URL: https://github.com/apache/arrow/pull/50687#issuecomment-5108113397
@lidavidm I'd appreciate some feedback on the approach here as you probably have a better mental model of how all this fits together.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #50687:
URL: https://github.com/apache/arrow/pull/50687#issuecomment-5107372288
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3667975739
##########
python/pyarrow/_flight.pyx:
##########
@@ -2887,6 +2887,7 @@ cdef class _FlightServerFinalizer(_Weakrefable):
status = server.Wait()
check_flight_status(status)
finally:
+ server.ReleasePythonServerRef()
Review Comment:
Without this the `test_flight_server_is_freed_on_exit` fails.
This seems to be called at `__exit__` too:
https://github.com/apache/arrow/blob/bda98c4d8c4dbf52c6b52186e07a7bb0578c0ac4/python/pyarrow/_flight.pyx#L3246-L3248
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3668014728
##########
python/pyarrow/src/arrow/python/flight.cc:
##########
@@ -86,6 +86,12 @@ PyFlightServer::PyFlightServer(PyObject* server, const PyFlightServerVtable& vta
server_.reset(server);
}
+void PyFlightServer::ReleasePythonServerRef() {
+ // Resets OwnedRefNoGIL to break the reference cycle between the C++ FlightServerBase
+ // and the Python object.
+ server_.reset();
Review Comment:
> right now, I think so, because it's called always after Shutdown which ensure the gRPC server is stopped:
Then it seems things are ok?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3668093032
##########
python/pyarrow/_flight.pyx:
##########
@@ -2887,6 +2887,7 @@ cdef class _FlightServerFinalizer(_Weakrefable):
status = server.Wait()
check_flight_status(status)
finally:
+ server.ReleasePythonServerRef()
Review Comment:
Commenting this line (`# server.ReleasePythonServerRef()`) the only test failing on `test_flight.py`:
```python
====================================================================================================== FAILURES ======================================================================================================
________________________________________________________________________________________ test_flight_server_is_freed_on_exit _________________________________________________________________________________________
def test_flight_server_is_freed_on_exit():
# Using FlightServerBase as a context manager should free
# the server object on exit.
with FlightServerBase('grpc://localhost:0') as server:
ref = weakref.ref(server)
del server
gc.collect()
> assert ref() is None
E AssertionError: assert <pyarrow._flight.FlightServerBase object at 0x7fcf6e7f5cb0> is None
E + where <pyarrow._flight.FlightServerBase object at 0x7fcf6e7f5cb0> = <weakref at 0x7fcf6e7f5bc0; to 'pyarrow._flight.FlightServerBase' at 0x7fcf6e7f5cb0>()
pyarrow/tests/test_flight.py:1153: AssertionError
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #50687:
URL: https://github.com/apache/arrow/pull/50687#issuecomment-5117773902
> How about we do:
>```python
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3668057264
##########
python/pyarrow/_flight.pyx:
##########
@@ -3234,6 +3235,7 @@ cdef class FlightServerBase(_Weakrefable):
raise ValueError("shutdown() on uninitialized FlightServerBase")
with nogil:
check_flight_status(self.server.get().Shutdown())
+ self.server.get().ReleasePythonServerRef()
Review Comment:
Commenting this line:
```python
====================================================================================================== FAILURES ======================================================================================================
____________________________________________________________________________________________ test_flight_server_is_freed _____________________________________________________________________________________________
def test_flight_server_is_freed():
# Calling server.shutdown manually should free the server object.
server = FlightServerBase('grpc://localhost:0')
server.shutdown()
server.wait()
ref = weakref.ref(server)
del server
gc.collect()
> assert ref() is None
E AssertionError: assert <pyarrow._flight.FlightServerBase object at 0x7fbb4dba7380> is None
E + where <pyarrow._flight.FlightServerBase object at 0x7fbb4dba7380> = <weakref at 0x7fbb4dba7650; to 'pyarrow._flight.FlightServerBase' at 0x7fbb4dba7380>()
pyarrow/tests/test_flight.py:1143: AssertionError
============================================================================================== short test summary info ===============================================================================================
FAILED pyarrow/tests/test_flight.py::test_flight_server_is_freed - AssertionError: assert <pyarrow._flight.FlightServerBase object at 0x7fbb4dba7380> is None
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3673186375
##########
python/pyarrow/tests/test_flight.py:
##########
@@ -1128,6 +1130,43 @@ def test_flight_server_location_argument():
assert isinstance(server, FlightServerBase)
+# The following tests are for GH-50684, which was a memory leak
+# in FlightServerBase.
+def test_flight_server_is_freed():
+ # Calling server.shutdown manually should free the server object.
+ server = FlightServerBase('grpc://localhost:0')
+ server.shutdown()
+ server.wait()
+ ref = weakref.ref(server)
+ del server
+ gc.collect()
+ assert ref() is None
+
+
+def test_flight_server_is_freed_on_exit():
+ # Using FlightServerBase as a context manager should free
+ # the server object on exit.
+ with FlightServerBase('grpc://localhost:0') as server:
+ ref = weakref.ref(server)
+ del server
+ gc.collect()
+ assert ref() is None
+
+
+@pytest.mark.xfail(
+ reason="GH-50684: FlightServerBase is not freed on delete without shutdown"
+)
+def test_flight_server_is_freed_without_shutdown():
+ # GH-50684: Not calling server.shutdown() currently leaks the server object.
+ # This test is expected to fail until the issue is fixed but is included
+ # for completeness and further discussion.
Review Comment:
I still have to finish grasping my head around this but while a weakref solves those lifetime issues we would introduce a deadlock.
From my understanding we still require a strong reference to the server for the dispatch methods in order to guarantee the server is present while serving.
If the Python server user reference disappears during the dispatch method execution, once the dispatch method finishes execution refcount reaches 0 and the finalizer runs on the serving thread, calling shutdown.
We seem to point that we should not call shutdown from a serving thread as this could block forever:
https://github.com/apache/arrow/blob/e1dfbefe2ebfe610609f4d718f652b910c966521/python/pyarrow/_flight.pyx#L3219-L3228
I can't see this prohibition on the C++ docs stated specifically but we hint that this blocks until requests are finished, from my understanding the current request won't finish because it is the one calling shutdown.
https://github.com/apache/arrow/blob/e1dfbefe2ebfe610609f4d718f652b910c966521/cpp/src/arrow/flight/server.h#L225-L232
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #50687:
URL: https://github.com/apache/arrow/pull/50687#issuecomment-5107462965
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #50687:
URL: https://github.com/apache/arrow/pull/50687#issuecomment-5128813613
Revision: 5b600c2c3841780a64af436d97760c604c87afbd
Submitted crossbow builds: [ursacomputing/crossbow @ actions-ed9acf3136](https://github.com/ursacomputing/crossbow/branches/all?query=actions-ed9acf3136)
|Task|Status|
|----|------|
|example-python-minimal-build-fedora-conda|[](https://github.com/ursacomputing/crossbow/actions/runs/30529421392/job/90827938016)|
|example-python-minimal-build-ubuntu-venv|[](https://github.com/ursacomputing/crossbow/actions/runs/30529419159/job/90827930258)|
|test-conda-python-3.11|[](https://github.com/ursacomputing/crossbow/actions/runs/30529417393/job/90827924362)|
|test-conda-python-3.11-dask-latest|[](https://github.com/ursacomputing/crossbow/actions/runs/30529419410/job/90827931594)|
|test-conda-python-3.11-dask-upstream_devel|[](https://github.com/ursacomputing/crossbow/actions/runs/30529418099/job/90827926764)|
|test-conda-python-3.11-hdfs-2.9.2|[](https://github.com/ursacomputing/crossbow/actions/runs/30529419702/job/90827931930)|
|test-conda-python-3.11-hdfs-3.2.1|[](https://github.com/ursacomputing/crossbow/actions/runs/30529420540/job/90827934958)|
|test-conda-python-3.11-hypothesis|[](https://github.com/ursacomputing/crossbow/actions/runs/30529420134/job/90827933457)|
|test-conda-python-3.11-pandas-2.0.3-numpy-1.23.2|[](https://github.com/ursacomputing/crossbow/actions/runs/30529419443/job/90827931400)|
|test-conda-python-3.11-spark-master|[](https://github.com/ursacomputing/crossbow/actions/runs/30529418808/job/90827929075)|
|test-conda-python-3.12|[](https://github.com/ursacomputing/crossbow/actions/runs/30529419602/job/90827931550)|
|test-conda-python-3.12-pandas-latest-numpy-latest|[](https://github.com/ursacomputing/crossbow/actions/runs/30529417846/job/90827926022)|
|test-conda-python-3.13|[](https://github.com/ursacomputing/crossbow/actions/runs/30529421866/job/90827939455)|
|test-conda-python-3.13-pandas-latest-numpy-1.26.2|[](https://github.com/ursacomputing/crossbow/actions/runs/30529418827/job/90827928878)|
|test-conda-python-3.13-pandas-latest-numpy-latest|[](https://github.com/ursacomputing/crossbow/actions/runs/30529418821/job/90827928883)|
|test-conda-python-3.14|[](https://github.com/ursacomputing/crossbow/actions/runs/30529420299/job/90827933888)|
|test-conda-python-3.14-cpython-debug|[](https://github.com/ursacomputing/crossbow/actions/runs/30529423408/job/90827944762)|
|test-conda-python-3.14-pandas-nightly-numpy-nightly|[](https://github.com/ursacomputing/crossbow/actions/runs/30529421259/job/90827937376)|
|test-conda-python-3.14-pandas-upstream_devel-numpy-nightly|[](https://github.com/ursacomputing/crossbow/actions/runs/30529418557/job/90827928348)|
|test-conda-python-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/30529418024/job/90827926519)|
|test-debian-13-python-3-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/30529420078/job/90827933023)|
|test-debian-13-python-3-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/30529418308/job/90827927302)|
|test-fedora-42-python-3|[](https://github.com/ursacomputing/crossbow/actions/runs/30529418801/job/90827928869)|
|test-ubuntu-24.04-python-3|[](https://github.com/ursacomputing/crossbow/actions/runs/30529420674/job/90827935049)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3667926659
##########
python/pyarrow/src/arrow/python/flight.cc:
##########
@@ -86,6 +86,12 @@ PyFlightServer::PyFlightServer(PyObject* server, const PyFlightServerVtable& vta
server_.reset(server);
}
+void PyFlightServer::ReleasePythonServerRef() {
+ // Resets OwnedRefNoGIL to break the reference cycle between the C++ FlightServerBase
+ // and the Python object.
+ server_.reset();
Review Comment:
> is this method only called after the gRPC server stopped running?
right now, I think so, because it's called always after Shutdown which ensure the gRPC server is stopped:
https://github.com/apache/arrow/blob/bda98c4d8c4dbf52c6b52186e07a7bb0578c0ac4/cpp/src/arrow/flight/server.cc#L206-L216
It is fair that we are not enforcing it here but we can't override `FlightServerBase::Shutdown` since it's not virtual so I am unsure how we could enforce it. Any ideas?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3676017597
##########
python/pyarrow/_flight.pyx:
##########
@@ -2887,6 +2887,7 @@ cdef class _FlightServerFinalizer(_Weakrefable):
status = server.Wait()
Review Comment:
Should I try removing it here? Probably not worth just creating a follow up issue for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3667701888
##########
python/pyarrow/_flight.pyx:
##########
@@ -3234,6 +3235,7 @@ cdef class FlightServerBase(_Weakrefable):
raise ValueError("shutdown() on uninitialized FlightServerBase")
with nogil:
check_flight_status(self.server.get().Shutdown())
+ self.server.get().ReleasePythonServerRef()
Review Comment:
Also, I'm curious: why have a C++ method that releases the Python ref, instead of simply releasing the `shared_ptr` here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3667678469
##########
python/pyarrow/src/arrow/python/flight.cc:
##########
@@ -86,6 +86,12 @@ PyFlightServer::PyFlightServer(PyObject* server, const PyFlightServerVtable& vta
server_.reset(server);
}
+void PyFlightServer::ReleasePythonServerRef() {
+ // Resets OwnedRefNoGIL to break the reference cycle between the C++ FlightServerBase
+ // and the Python object.
+ server_.reset();
Review Comment:
Is it possible that a C++ Flight server method such as `PyFlightServer::ListFlights` runs after this (for example from another thread) and crashes when trying to dereference `server_`?
In other words, is this method only called after the gRPC server stopped running?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3674545318
##########
python/pyarrow/_flight.pyx:
##########
@@ -2887,6 +2887,7 @@ cdef class _FlightServerFinalizer(_Weakrefable):
status = server.Wait()
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #50687:
URL: https://github.com/apache/arrow/pull/50687#issuecomment-5105780966
Revision: 6c7c46a5f05894690dbeefda11f95d23f9dd2955
Submitted crossbow builds: [ursacomputing/crossbow @ actions-e80f3b2c61](https://github.com/ursacomputing/crossbow/branches/all?query=actions-e80f3b2c61)
|Task|Status|
|----|------|
|wheel-manylinux-2-28-cp313-cp313-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/30370765461/job/90313701800)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #50687:
URL: https://github.com/apache/arrow/pull/50687#issuecomment-5118014717
moving into review to correctly reflect state as we have already been reviewing it :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #50687:
URL: https://github.com/apache/arrow/pull/50687#issuecomment-5156442096
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 9ec70551b9caa042f15c475e1fe0a611bb7002ba.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/91473527130) has more details. It also includes information about 34 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #50687:
URL: https://github.com/apache/arrow/pull/50687#issuecomment-5128776218
@github-actions crossbow submit -g python
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #50687:
URL: https://github.com/apache/arrow/pull/50687#issuecomment-5117372370
Here's an idea: instead of:
```python
def shutdown(self):
with nogil:
check_flight_status(self.server.get().Shutdown())
self.server.get().ReleasePythonServerRef()
```
How about we do:
```python
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #50687:
URL: https://github.com/apache/arrow/pull/50687#issuecomment-5117839152
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3673826598
##########
python/pyarrow/tests/test_flight.py:
##########
@@ -1128,6 +1130,43 @@ def test_flight_server_location_argument():
assert isinstance(server, FlightServerBase)
+# The following tests are for GH-50684, which was a memory leak
+# in FlightServerBase.
+def test_flight_server_is_freed():
+ # Calling server.shutdown manually should free the server object.
+ server = FlightServerBase('grpc://localhost:0')
+ server.shutdown()
+ server.wait()
+ ref = weakref.ref(server)
+ del server
+ gc.collect()
+ assert ref() is None
+
+
+def test_flight_server_is_freed_on_exit():
+ # Using FlightServerBase as a context manager should free
+ # the server object on exit.
+ with FlightServerBase('grpc://localhost:0') as server:
+ ref = weakref.ref(server)
+ del server
+ gc.collect()
+ assert ref() is None
+
+
+@pytest.mark.xfail(
+ reason="GH-50684: FlightServerBase is not freed on delete without shutdown"
+)
+def test_flight_server_is_freed_without_shutdown():
+ # GH-50684: Not calling server.shutdown() currently leaks the server object.
+ # This test is expected to fail until the issue is fixed but is included
+ # for completeness and further discussion.
Review Comment:
I've reproduced the hang with the following small snippet and a local PoC moving to a weakref, done is never printed, it hangs forever:
```python
import threading
import time
import pyarrow as pa
import pyarrow.flight as fl
class Server(fl.FlightServerBase):
def do_get(self, context, ticket):
time.sleep(2)
return fl.RecordBatchStream(pa.table({"a": [1]}).to_reader())
server = Server("grpc://127.0.0.1:0")
location = f"grpc://127.0.0.1:{server.port}"
def client():
fl.FlightClient(location).do_get(fl.Ticket(b"")).read_all()
thread = threading.Thread(target=client)
thread.start()
time.sleep(1)
del server
thread.join()
print("done")
```
With py-spy dump I can see the idle thread and part of the stack pointing to the DoGet thread calling shutdown, this is trimmed:
```gdb
grpc_completion_queue_next (completion_queue.cc:1154)
grpc::CompletionQueue::AsyncNextInternal (completion_queue_cc.cc:147)
grpc::CompletionQueue::AsyncNext<gpr_timespec> (completion_queue.h:202)
grpc::Server::ShutdownInternal (server_cc.cc:1296)
grpc::ServerInterface::Shutdown (server_interface.h:106)
Shutdown (grpc_server.cc:649)
arrow::flight::FlightServerBase::Shutdown (server.cc:216)
_FlightServerFinalizer_finalize (pyarrow/_flight.pyx:2885)
finalize (pyarrow/_flight.pyx:2877)
__Pyx_CyFunction_Vectorcall_FASTCALL_KEYWORDS (pyarrow/_flight.pyx:1666)
__call__ (weakref.py:590)
__pyx_tp_dealloc_7pyarrow_3lib__Weakrefable (pyarrow/public-api.pxi:439)
__pyx_tp_dealloc_7pyarrow_7_flight_FlightServerBase (pyarrow/_flight.pyx:3251)
Py_XDECREF (object.h:1049)
arrow::py::OwnedRef::reset (common.h:202)
arrow::py::OwnedRef::reset (common.h:205)
arrow::py::OwnedRef::~OwnedRef (common.h:198)
operator() (flight.cc:160)
SafeCallIntoPython<arrow::py::flight::PyFlightServer::DoGet(const arrow::flight::ServerCallContext&, const arrow::flight::Ticket&, std::unique_ptr<arrow::flight::FlightDataStream>*)::<lambda()> > (common.h:161)
arrow::py::flight::PyFlightServer::DoGet (flight.cc:161)
arrow::flight::internal::ServerTransport::DoGet (transport_server.cc:301)
DoGet (grpc_server.cc:493)
operator() (Flight.grpc.pb.cc:300)
__invoke_impl<grpc::Status, arrow::flight::protocol::FlightService::Service::Service()::<lambda(arrow::flight::protocol::FlightService::Service*, grpc::ServerContext*, const arrow::flight::protocol::Ticket*, grpc::ServerWriter<arrow::flight::protocol::FlightData>*)>&, arrow::flight::protocol::FlightService::Service*, grpc::ServerContext*, const arrow::flight::protocol::Ticket*, grpc::ServerWriter<arrow::flight::protocol::FlightData>*> (invoke.h:63)
```
This is not happening today because the strong reference enforces refcount > 0, we have to manually call shutdown for the reference to be released which should never be done from a serving thread as the method documents. I am now wondering whether not calling shutdown and keeping the Python object alive should be the intended behavior instead of being treated as a bug leaking an object on this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3674679374
##########
python/pyarrow/tests/test_flight.py:
##########
@@ -1128,6 +1130,43 @@ def test_flight_server_location_argument():
assert isinstance(server, FlightServerBase)
+# The following tests are for GH-50684, which was a memory leak
+# in FlightServerBase.
+def test_flight_server_is_freed():
+ # Calling server.shutdown manually should free the server object.
+ server = FlightServerBase('grpc://localhost:0')
+ server.shutdown()
+ server.wait()
Review Comment:
yes, it does work, `wait` is not required, probably just a remnant from previous local trials. I will remove.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3674553947
##########
python/pyarrow/tests/test_flight.py:
##########
@@ -1128,6 +1130,43 @@ def test_flight_server_location_argument():
assert isinstance(server, FlightServerBase)
+# The following tests are for GH-50684, which was a memory leak
+# in FlightServerBase.
+def test_flight_server_is_freed():
+ # Calling server.shutdown manually should free the server object.
+ server = FlightServerBase('grpc://localhost:0')
+ server.shutdown()
+ server.wait()
Review Comment:
Does this work if we don't call `wait`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50684: [Python][FlightRPC] Break the reference cycle between the C++ FlightServerBase and the Python object to avoid leaking server [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50687:
URL: https://github.com/apache/arrow/pull/50687#discussion_r3667691487
##########
python/pyarrow/_flight.pyx:
##########
@@ -2887,6 +2887,7 @@ cdef class _FlightServerFinalizer(_Weakrefable):
status = server.Wait()
check_flight_status(status)
finally:
+ server.ReleasePythonServerRef()
Review Comment:
Are you sure this actually works? `finalize` is only called when the Python `FlightServerBase` object has been destroyed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org