docs(coding-guidelines): Add request response correlation #3290
Conversation
Allow correlating asynchronous responses to their requests.
There was a problem hiding this comment.
Thanks for writing this up!
I think we should settle on one approach on how we do this in the codebase, otherwise it is not really a guideline.
My preference would be client-side generated IDs for commands:
They are a special case of "user data" just not being configurable, thus they can carry the same usecases but without the downsides of type parameters which make the code complex.
I am not sure about methods. For #3292, we would need client-side generated IDs but we can't do that if we have a more low-level component that hands out IDs.
|
We might also want to add that this only applies to certain things. Streams for example should not have an identity but be interchangable in my opinion. See #3268. |
I am assuming that you want to decide on either userdata or user-provided-ids, correct? I don't feel strongly that we should be doing either or. In other words, I am fine using both in the rust-libp2p codebase, depending on the use-case.
Thanks for bringing this up. for |
Yes, I'd very much appreciate consistency here. I think it will make it easier to understand the codebase and allow us to better settle on reusable components.
Ugh, not sure about two IDs 🫤 I think I'd prefer if we require the user to pass a |
There was a problem hiding this comment.
Thanks for writing this up @mxinden.
Right now, this describes different approaches to solve more or less the same thing. I think it would be useful if there would be some additional notes on when it makes sense to use what.
docs/coding-guidelines.md
Outdated
| ``` rust | ||
| fn my_method() -> Id { | ||
| // ... | ||
| } | ||
| ``` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I don't have a good rule here, nor an opinion. Thus far I was simply deciding case-by-case.
Note that this is only really valid once we have libp2p#3327 and the corresponding patch in `libp2p-swarm` `DialOpts`.
|
a951fbd and 694a09c remove the user-data option. Thanks for pushing for it @thomaseizinger and @elenaf9. |
docs/coding-guidelines.md
Outdated
| 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 { | ||
| // ... | ||
| } | ||
| ``` |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
|
Addressed latest comment. @thomaseizinger please add the |
Description
Allow correlating asynchronous responses to their requests.
Notes
Links to any relevant issues
Came up in discussions around #2824.
Open Questions
Change checklist
I have added tests that prove my fix is effective or that my feature worksA changelog entry has been made in the appropriate crates