Sitelet https://web.archive.org/web/20201109055022/https://github.com/google/tarpc/issues/321
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Server shuts down before it has finished processing a client request #321

Open
faern opened this issue Oct 21, 2020 · 12 comments
Open

Server shuts down before it has finished processing a client request #321

faern opened this issue Oct 21, 2020 · 12 comments
Labels

Comments

@faern
Copy link

@faern faern commented Oct 21, 2020 •

I took the example crate from this repository, example-service, and started converting it towards using tarpc::serde_transport::new. I want to learn how to use this crate in a transport agnostic way[1].

I then changed the server to only accept a single TCP connection and create a tarpc server for that single connection[2]. I took inspiration from the server example code in the main crate documentation using .incoming(stream::once(future::ready(server_transport))).

I ended up with this: https://github.com/faern/tarpc/blob/a1ffbec2a8c7c6ebbd3c5701f1654ee13829cf6d/example-service/src/server.rs.

    let server = server::new(server::Config::default())
        .incoming(stream::once(future::ready(server_transport)))
        .respond_with(HelloServer(client_addr).serve());

    server.await;

However the server would die and the client would print Error: Kind(ConnectionReset). I realized server.await returned before it was done processing the client request. Adding an ugly sleep made it work again: faern@0ac9f34#diff-31d1194bbeab9b1b6afdc652567b5a0193287d4e8bba43bc13b4ccbb648320d0.

Another way to hack around it is with this solution: https://github.com/faern/tarpc/blob/9e9f6306f54c524557829a43fffa1029e2edea1f/example-service/src/server.rs#L68-L74

    let channel = server::new(server::Config::default())
        .incoming(stream::once(future::ready(server_transport)))
        .next()
        .await
        .expect("This server should have exactly one channel in it");

    channel.respond_with(HelloServer(client_addr).serve()).execute().await;

I think the first type of solution (the one not working properly) is the cleaner from the aspect of consuming tarpc. I don't need to pull out a single Channel out of the stream with next() etc. I just give it a single transport and tell it to process that entire transport. The problem is that it does not process my entire transport, it aborts prematurely.

[1]: I'm ultimately going to use this over virtual serial ports and domain sockets. So personally I would love if this crate had an abstraction level where it could be used without any predefined transport being mixed in.

[2]: My use case is to host an RPC server in a virtual machine where some hypervisor administrator process will talk to it over a virtual serial port. As a result there will only ever be one "client" to this RPC server (the serial port file). I would love if the crate allowed expressing this in a nice way.

@faern
Copy link
Author

@faern faern commented Oct 21, 2020

Or maybe I have just greatly misunderstood how to use the library. Any input is greatly appreciated! :)

@tikue
Copy link
Collaborator

@tikue tikue commented Oct 21, 2020

Hey, thanks for the interest! Quick plug of our discord channel, which is good for longer-form discussions.

  • Re: [1], tarpc is transport agnostic, so you are free to plug in whatever transport you're using. Ultimately you just need something that impls Stream<Item = io::Result<tarpc::ClientMessage<Request>>> + Sink<tarpc::Response<Response>, Error = io::Error> that you can pass to BaseChannel::new.
  • Re: [2], your server.await only waits until the stream of incoming connections is closed. Handler::respond_with spawns each channel onto the tokio default executor. Once the channels are spawned, the executor still needs to execute them. But since you only have one client, I would entirely avoid using server::new, which is really just a small shim to help serve a stream of channels. Channel represents a server connection to a client and can be used by itself (see above bullet).

Try this:

BaseChannel::with_defaults(server_transport)
    .respond_with(HelloServer(client_addr).serve())
    .execute()
    .await;
@tikue tikue added the question label Oct 21, 2020
@tikue
Copy link
Collaborator

@tikue tikue commented Oct 21, 2020

By the way, I'd appreciate any PRs to make the documentation more helpful!

@faern
Copy link
Author

@faern faern commented Oct 22, 2020

Awesome! Thanks. Fewer types involved and fewer hacks needed (stream::once(future::ready(server_transport))). That is indeed a cleaner way to handle a single connection. I'll try it out later. Maybe I'll come by the discord channel if I run into more problems :)

@tikue
Copy link
Collaborator

@tikue tikue commented Oct 23, 2020

Hey @faern, anything else you need help with? Can I close this issue?

@faern
Copy link
Author

@faern faern commented Oct 24, 2020

Thanks for the new example. That's awesome.

I still think there is some documentation that can be improved. But I have not had the time to play more with this since you provided additional information.

Channel::respond_with:

Respond to requests coming over the channel with f. Returns a future that drives the responses and resolves when the connection is closed.

It seems this documentation is not entirely true since the future completes before the response are fully completed? And it's not immediately clear to me that "connection is closed" refers to the listening server socket and not all the client sockets.

@faern
Copy link
Author

@faern faern commented Oct 24, 2020

The discord invite does not work. Maybe it expired or I'm not good enough at understanding discord 😅

Do you link to the discord anywhere except randomly in issues? It's not in the main readme. It would be a great way to find the community.

@tikue
Copy link
Collaborator

@tikue tikue commented Oct 24, 2020

There's a discord badge in the readme 🙂

@tikue
Copy link
Collaborator

@tikue tikue commented Oct 24, 2020

And it's not immediately clear to me that "connection is closed" refers to the listening server socket and not all the client sockets.

This refers to the one client connection managed by the channel. It does not refer to a server listening on a socket.

@faern
Copy link
Author

@faern faern commented Oct 24, 2020

Thanks! Badges does not work for ctrl-f disco..., chat or community :D

@faern
Copy link
Author

@faern faern commented Oct 24, 2020

This refers to the one client connection managed by the channel. It does not refer to a server listening on a socket.

But you just said the opposite:

your server.await only waits until the stream of incoming connections is closed

.. Or I'm too tired :D

@tikue
Copy link
Collaborator

@tikue tikue commented Oct 24, 2020

I think the confusion is that there are multiple respond_with fns, one on Channel and one on server.incoming(). The one on server.incoming() is the one that only waits until server.incoming() is closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.