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

[WIP] Rust client #47

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

[WIP] Rust client #47

wants to merge 18 commits into from

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Apr 24, 2018

This PR introduces a Rust client for the cluster with a synchronous API that mirrors the Python client.

The client is used to implement a stop command for the cluster. The terminateServer RPC call on the scheduler is now implemented with a trivial exit(0), later it should probably do a more graceful shutdown.

I'm going to need help with my arch-nemesis (storing closures in structs) to somehow implement Drop for Session (which needs access to the client).

@spirali
Copy link
Collaborator

spirali commented Apr 25, 2018

I am quite confused what you want to achieve and I have feeling that storing closures in structs is not the right thing. After quick review, it seems to me that you need an need an Rc to some inner structure that is shared by client and session and contains communication structures, alternative may be also creating a sending channel (MPSC) and store sender of this channel in sessions and client).

@Kobzol
Copy link
Contributor Author

Kobzol commented Apr 25, 2018

You're right, I wanted to avoid creating a shared inner state structure, but I guess it cannot be avoided.

@spirali
Copy link
Collaborator

spirali commented Apr 25, 2018

Why you wanted to avoid it?

@Kobzol
Copy link
Contributor Author

Kobzol commented Apr 25, 2018

Well it's a minor complication for the code, but it's fully justified in this case. I wanted to know if there's some Rust way to do it without the shared state (for example somehow binding the lifetimes of Session and Client together, so that the compiler would check that Session cannot outlive the Client).

@spirali
Copy link
Collaborator

spirali commented Apr 25, 2018

You can store a reference to the client in a session (with lifetime templates) and then compiler do the checks. But it would be little bit complicated to use such session e.g. in futures where you cannot simply prove that something lives sufficiently long.

@Kobzol
Copy link
Contributor Author

Kobzol commented Apr 26, 2018

I refactored it using the WrappedRcRefCell. I have two (maybe noob) questions.

  1. I made the Client outside API surface shared (the client's methods use &self, not &mut self). Since it uses internal mutability using the WrappedRcRefCell and it can't be sent to another threads, is this safe?
  2. The WrappedRcRefCell uses self.inner.clone() for cloning the Rc. I noticed that this can deep clone the inner object (https://github.com/nical/rfcs/blob/rc-newref-clone/text/0000-rc-newref-clone.md), isn't Rc::clone(&self.inner) a better option?
    EDIT: ignore 2), I understood it wrong, they propose to use Rc::clone(&inner) for readability purposes, not performance

I thought that my session close method didn't work because the session stayed in the dashboard. But then I realized that the dashboard doesn't clear the sessions. It should probably mark the closed sessions somehow (since they cannot be reopened).

@spirali
Copy link
Collaborator

spirali commented Apr 26, 2018

(1) It is ok, mut/non-mut in Rust is different concept than C++ const/non-cost. If it compiles and you do not use "unsafe" then it is safe:)
(3) Dashboard is quite unfinished, so you are completely right, it should mark closed sessions. For debugging, logs are more reliable. Do not forget to set env variable RUST_LOG="debug".

@Kobzol
Copy link
Contributor Author

Kobzol commented Apr 26, 2018

I was worried about a possible situation where the refcell would be borrowed mutably multiple times at once, but I don't think that's possible with the current design (since the rc is private to the client).

Now I will wait for the Rust subworker and then I'll try to integrate it with the client to submit tasks from Rust.

@spirali
Copy link
Collaborator

spirali commented Apr 26, 2018

It is checked at runtime; multiple borrows causes panic, so it safe.

If you want to continue, you can wrap build-in functions (e.g. concat). However, it still makes sense to wait since we want to separated some common structures into a separate crate.

@Kobzol
Copy link
Contributor Author

Kobzol commented May 2, 2018

Added basic tasks, data objects and fetch wrappers. The contents of capnp.rs should probably be using the serialization machinery from common.

Example usage:

let client = Client::new(SocketAddr::new(
    IpAddr::from_str("127.0.0.1")?,
    7210,
))?;
let mut session = client.new_session()?;

let hello = session.blob("Hello ".into());
let world = session.blob("world!".into());
let task = session.concat(vec![hello, world]);
let output = task.get().output()?;
output.get_mut().keep();

session.submit()?;

let result = String::from_utf8(session.fetch(&output.get())?)?;
println!("{}", result); // Hello world!

@Kobzol
Copy link
Contributor Author

Kobzol commented May 3, 2018

Added local cluster with locally spawned processes (using Starter).

@Kobzol Kobzol force-pushed the rust-client branch 2 times, most recently from 34833b0 to 3899a9e Compare May 4, 2018 11:22
@Kobzol Kobzol mentioned this pull request May 4, 2018
@Kobzol Kobzol force-pushed the rust-client branch 2 times, most recently from 7ea984b to 780ed93 Compare July 7, 2018 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants