Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions python/pyarrow/_flight.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -2883,10 +2883,9 @@ cdef class _FlightServerFinalizer(_Weakrefable):
try:
with nogil:
status = server.Shutdown()
if status.ok():
status = server.Wait()
check_flight_status(status)
finally:
server.ReleasePythonServerRef()
Comment thread
pitrou marked this conversation as resolved.
self.server.reset()


Expand Down Expand Up @@ -3234,6 +3233,7 @@ cdef class FlightServerBase(_Weakrefable):
raise ValueError("shutdown() on uninitialized FlightServerBase")
with nogil:
check_flight_status(self.server.get().Shutdown())
self.server.get().ReleasePythonServerRef()
Comment thread
pitrou marked this conversation as resolved.

def wait(self):
"""Block until server is terminated with shutdown."""
Expand Down
1 change: 1 addition & 0 deletions python/pyarrow/includes/libarrow_flight.pxd
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,7 @@ cdef extern from "arrow/python/flight.h" namespace "arrow::py::flight" nogil:
CStatus ServeWithSignals() except *
CStatus Shutdown()
CStatus Wait()
void ReleasePythonServerRef()

cdef cppclass PyServerAuthHandler\
" arrow::py::flight::PyServerAuthHandler"(CServerAuthHandler):
Expand Down
6 changes: 6 additions & 0 deletions python/pyarrow/src/arrow/python/flight.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Comment thread
pitrou marked this conversation as resolved.
}

Status PyFlightServer::ListFlights(
const arrow::flight::ServerCallContext& context,
const arrow::flight::Criteria* criteria,
Expand Down
3 changes: 3 additions & 0 deletions python/pyarrow/src/arrow/python/flight.h
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,9 @@ class ARROW_PYFLIGHT_EXPORT PyFlightServer : public arrow::flight::FlightServerB
Status ListActions(const arrow::flight::ServerCallContext& context,
std::vector<arrow::flight::ActionType>* actions) override;

// Breaks the reference cycle between the C++ FlightServerBase and the Python object.
void ReleasePythonServerRef();

private:
OwnedRefNoGIL server_;
PyFlightServerVtable vtable_;
Expand Down
42 changes: 40 additions & 2 deletions python/pyarrow/tests/test_flight.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@

import ast
import base64
from datetime import datetime
import gc
import itertools
import json
import os
import pathlib
import signal
Expand All @@ -28,8 +31,7 @@
import threading
import time
import traceback
import json
from datetime import datetime
import weakref

try:
import numpy as np
Expand Down Expand Up @@ -1128,6 +1130,42 @@ 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()
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.
Comment thread
pitrou marked this conversation as resolved.
server = FlightServerBase('grpc://localhost:0')
ref = weakref.ref(server)
del server
gc.collect()
assert ref() is None


def test_server_exit_reraises_exception():
with pytest.raises(ValueError):
with FlightServerBase():
Expand Down
Loading