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

Handler track request/reply #2

Open
vlopes11 opened this issue Apr 23, 2019 · 6 comments
Open

Handler track request/reply #2

vlopes11 opened this issue Apr 23, 2019 · 6 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@vlopes11
Copy link
Owner

Minimal solution

  1. Create a new attribute hm_requests of JrpcHandler as HashMap<String, Implementation of JrpcMethodTrait>

    • Use hm_methods as reference
  2. Create a new method register_request of JrpcHandler that will receive an implementation of ToString and a future, and will register this information to hm_requests

    • Use register_method as reference
    • The identifier of the request, most likely as Uuid, will be the key for the HashMap
  3. Create a new method wait_for_request_reply of JrpcHandler that will receive one implementation of JrpcRequest and a future

    • This method will insert a new track in hm_requests via the created register_request method using the get_id method of JrpcRequest
    • If get_id is None, this is not a valid request, according to the JsonRpc specification. Therefore, we need a new ErrorVariant for that
  4. Change handle_message method of JrpcHandler to check if the parsed message Json contains method attribute.

    1. If yes, then use current logic of fetching the future from hm_methods.
    2. If no, then implement a new logic to fetch the future from hm_requests, and delete the HashMap associated register after the retrieval of the generated future

Optimal solution

  • Test cases
  • Mechanism to timeout old requests in hm_requests
@vlopes11 vlopes11 added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Apr 23, 2019
@LupusAnay
Copy link

Hi.

I'm new to programming on rust, and if it's possible, I'd like to receive answers to some (may be stupid) questions about this issue.

ID attribute

I'm not very familiar with JSON-RPC, but quick look to the spec told me, that

If get_id is None, this is not a valid request, according to the JsonRpc specification

not quite right. JSON-RPC 2.0 says:

A Notification is a Request object without an "id" member.

Method attribute

Same question about

check if the parsed message Json contains method attribute

According to the specification, there can be no valid request without a method attribute.
(if I correctly understood that the absence of an explicit indication that the parameter can be omitted means that the parameter is mandatory)

So, if it doesn’t bother you, then I would like to hear the answers to these questions (and maybe later some others, also may be stupid).

@vlopes11
Copy link
Owner Author

Hello!

Hey, nothing is stupid. It is very positive that you read the JSON-RPC specification and now have questions about it.

ID attribute

I'm not very familiar with JSON-RPC, but a quick look to the spec told me, that

If get_id is None, this is not a valid request, according to the JSON-RPC specification

not quite right. JSON-RPC 2.0 says:

A Notification is a Request object without an "id" member.

This change aims to send a request and receive a response to the process with a future.

According to the specification, if the request will not contain id, then it will be a notification.

If it is a notification, then we should not track it in the futures queue, because there will never be a reply for it. To keep it possible to track request x response, it is mandatory we have an id.

If the user wants to send a notification, he can use the following method:

pub fn prepare_to_send_notification<T: ToString>(

This method will return a raw notification message.

Method attribute

Same question about

check if the parsed message JSON contains method attribute

According to the specification, there can be no valid request without a method attribute.
(if I correctly understood that the absence of an explicit indication that the parameter can be omitted means that the parameter is mandatory)

The message handler

pub fn handle_message<T: ToString>(

Will receive anything. It will need a way to classify if the incoming message is a reply from a previous request or a new request. If the method attribute is mandatory for a request, then, if we don't have it, necessarily we received a response object.

@LupusAnay
Copy link

Hi again, sorry for big gap between messages.

Thank you for your answer. Should I create new type for parsing message without method, or i should just make JrpcRequest's method attribute Option?

And it's not clear for me with

an implementation of ToString and a future

what does this "future" mean? It's just generic Future trait, or Future, that returned by generate_future method. And if it's a future, why hm_requests have a

HashMap<String, Implementation of JrpcMethodTrait>

type?
What's the process stand behind making from "future" the "Implementation of JrpcMethodTrait"?

I'm not sure, if I'm suppose to understand this by myself. I've read your code of this project a couple time, and it seemed clear for me, but this task is quite hard to understand.
So if this is not a big problem for you, and you have time to explain all these stuff, that would be cool.

@vlopes11
Copy link
Owner Author

vlopes11 commented May 6, 2019

Hello!

The proposed solution is indeed not very strict in a way we can maybe bring different perspectives.

Thank you for your answer. Should I create new type for parsing message without method, or i should just make JrpcRequest's method attribute Option?

JrpcRequest need to have the method attribute as mandatory, to cope with the specification.

To avoid the creation of a specific type only to check that, we can use standard serde_json to solve this situation. Here is one example:

use serde_json::{json, Value};

fn main() {
    let js = json!({
        "some_attribute": "some value",
        "other": 43,
    });

    if let Value::Number(n) = &js["other"] {
        println!("Other exist and is {}", n);
    }

    if let Value::Null = &js["method"] {
        println!("Method does not exist");
    }

    if let None = js.get("method") {
        println!("Also other way");
    }
}

And it's not clear for me with

an implementation of ToString and a future

what does this "future" mean? It's just generic Future trait, or Future, that returned by generate_future method. And if it's a future, why hm_requests have a

HashMap<String, Implementation of JrpcMethodTrait>

type?
What's the process stand behind making from "future" the "Implementation of JrpcMethodTrait"?

Here, we can mirror the behavior of register_method

pub fn register_method<T: ToString, F: JrpcMethodTrait<'a> + 'a>(

We receive an implementation of ToString trait to act as an identifier for the request.

As for the future, it's less strict in the definition because we can either receive a concrete implementation of Future, or follow the standard for the requests and receive an implementation of JrpcMethodTrait.

The advantage of using JrpcMethodTrait is the ability to generate as many concrete handlers as we want/need, and store some particular data to each of these.

But, in this case, we need only one future with any data we store there. It's like this because a registered request will have one, and only one reply.

This means we don't really have an advantage of using JrpcMethodTrait but to follow the same standard as register_method.

If we opt to use JrpcMethodTrait, then, in handle_message, we call the generate_future to return the handler for that request.

If we opt to store only a Future implementation, then we just return it.

What do you think is the best option?

@alfredoyang
Copy link

Hi,
I don't understand the goal of this issue.
From issue description:

4. Change `handle_message` method of `JrpcHandler` to check if the parsed message Json contains `method` attribute.
   
   1. If yes, then use current logic of fetching the future from `hm_methods`.
   2. If no, then implement a new logic to fetch the future from `hm_requests`, and delete the `HashMap` associated register after the retrieval of the generated future

Is it a valid JSON RPC call without method member?

@vlopes11
Copy link
Owner Author

Hello, alfredo,

Via the same interface, we will receive anything, including replies and requests. To differentiate between these two, we check for the method member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants