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

docs(coding-guidelines): Add request response correlation #3290

Merged
merged 6 commits into from
Feb 27, 2023
Merged
Changes from 4 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
40 changes: 40 additions & 0 deletions docs/coding-guidelines.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
- [Further Reading](#further-reading)
- [Use iteration not recursion](#use-iteration-not-recursion)
- [Further Reading](#further-reading-1)
- [Allow Correlating Asynchronous Responses to Their Requests](#allow-correlating-asynchronous-responses-to-their-requests)

<!-- markdown-toc end -->

Expand Down Expand Up @@ -299,3 +300,42 @@ stack potentially unboundedly. Instead use iteration e.g. via `loop` or `for`.
- https://en.wikipedia.org/wiki/Tail_call
- https://stackoverflow.com/questions/65948553/why-is-recursion-not-suggested-in-rust
- https://stackoverflow.com/questions/59257543/when-is-tail-recursion-guaranteed-in-rust

## Allow Correlating Asynchronous Responses to Their Requests

In an asynchronous context, it is important to enable users to determine the correlation between a
response and a previous request. For example, if a user requests two new connections to the same
peer, they should be able to match each new connection to the corresponding previous connection
request without having to guess.
elenaf9 marked this conversation as resolved.
Show resolved Hide resolved

When providing a **method** where the response to that method is delivered asynchronously through an
event, either synchronously return a request ID which is later on contained in the asynchronous
response event, or synchronously return a `Future` that eventually resolves into the response.

``` rust
fn my_method() -> Id {
// ...
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Compared to the Command below the ID here is not user generated but instead handed out to the user. When would it be recommended to use user-generated IDs and when should they be handed out?

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 don't have a good rule here, nor an opinion. Thus far I was simply deciding case-by-case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to remove this and be consistent with commands where the caller always specifies the ID? Only Transport::dial currently behaves like stated here and we already said we wanted to fix this! :)

I'd be in favor of removing this from the guidelines (or rather, say that we want to accept the ID) and opening an issue to fix Transport::dial.

Copy link
Member Author

Choose a reason for hiding this comment

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

with commands where the caller always specifies the ID?

I have to give this more thought. In other words, I am not sure this is the way to go in all situations.

Do you feel strongly about removing this @thomaseizinger? I would prefer proceeding here. I see this pull request as a net-positive at this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense for anything that is public API in our library. Internally, where we control all usages of an ID, it doesn't matter as much.

The reason for that is that the majority of our public API is command-based. Anything command-based needs to create the ID ahead of time. Any component underneath that creates its own IDs causes friction here because it doesn't compose well with commands.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit torn. There is certainly value in documenting this but if we document something, I'd prefer if we can back the written words 100%, e.g. have strong convictions that this is the way to go. Everything else is just confusing to the reader.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would removing this section be a middle ground? i.e. only document that commands should come with a user-generated ID and not talk about functions at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done with 12e61b6.


``` rust
fn my_method() -> impl Future<Output = Response> {
// ...
}
elenaf9 marked this conversation as resolved.
Show resolved Hide resolved
```

When accepting a **command** that eventually results in a response through an event require that
command to contain a unique ID, which is later on contained in the asynchronous response event. One
such example is the `Swarm` accepting a `NetworkBehaviourAction::Dial` from the `NetworkBehaviour`.

``` rust
struct Command {
id: Id,
// ...
}

struct Response {
command_id: Id,
// ...
}
```