Skip to content

Conversation

@lidavidm
Copy link
Member

This uses a stop token to let interactive users interrupt a long-running Flight operation. It's not perfect: the operation won't be cancelled until the server delivers a message, so this doesn't protect against very slow servers. (In that case, we'd need some way for the stop source to call TryCancel() on the gRPC RPC object, which would be tricky.) But so long as the server is being responsive, this means Ctrl-C should do what people expect in Python.

@github-actions
Copy link

@cyb70289
Copy link
Contributor

Not quite sure of the issue addressed.
Is it possible to reproduce the issue on my local host and verify the fix?

@lidavidm
Copy link
Member Author

It solves a problem for Python users: if you Ctrl-C in something like read_all(), nothing will happen because control is in C++/Flight code, and then you're stuck waiting for the call to complete.

If you apply this diff to python/examples/flight/server.py:

diff --git a/python/examples/flight/server.py b/python/examples/flight/server.py
index 7a6b6697e..bc1df21d1 100644
--- a/python/examples/flight/server.py
+++ b/python/examples/flight/server.py
@@ -73,6 +73,7 @@ class FlightServer(pyarrow.flight.FlightServerBase):
             yield self._make_flight_info(key, descriptor, table)
 
     def get_flight_info(self, context, descriptor):
+        return self._make_flight_info("", descriptor, pyarrow.schema([]).empty_table())
         key = FlightServer.descriptor_to_key(descriptor)
         if key in self.flights:
             table = self.flights[key]
@@ -86,10 +87,10 @@ class FlightServer(pyarrow.flight.FlightServerBase):
         print(self.flights[key])
 
     def do_get(self, context, ticket):
-        key = ast.literal_eval(ticket.ticket.decode())
-        if key not in self.flights:
-            return None
-        return pyarrow.flight.RecordBatchStream(self.flights[key])
+        import itertools
+        schema = pyarrow.schema([])
+        rb = pyarrow.RecordBatch.from_arrays([], schema=schema)
+        return pyarrow.flight.GeneratorStream(schema, itertools.repeat(rb))
 
     def list_actions(self, context):
         return [

Then you can start the server:

arrow/python$ env PYTHONPATH=$(pwd) python examples/flight/server.py --port 2000 &
arrow/python$ env PYTHONPATH=(pwd) python examples/flight/client.py get localhost:2000 -c foo

Without this patch, if you Ctrl-C, your client will still be stuck forever. With this patch you'll interrupt the call as expected.

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

This is really nice!

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for the Table version to be virtual? It seems it can simply be implemented in the base class...?

Of course, it doesn't matter much as long as we have only one implementation of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I made this non-virtual.

Copy link
Member

Choose a reason for hiding this comment

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

Am I reading this wrong, or is this twice the same condition?

Copy link
Member Author

Choose a reason for hiding this comment

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

I meant for the second to be KeyboardInterrupt.

Copy link
Member

Choose a reason for hiding this comment

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

Would this raise an exception if the context is cancelled, or just succeed immediately?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should raise an exception, but either way, the client is gone at this point so there's not much the server can do.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I was just wondering if the server could end up spending more time than necessary here :-)

@pitrou
Copy link
Member

pitrou commented Jun 2, 2021

As a sidenote, I think we'll need to allow adding a callback on a StopToken, so that arbitrary callables such as gRPC's TryCancel can be called. I opened a JIRA for it: https://issues.apache.org/jira/browse/ARROW-12938

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants