docs/coding-guidelines: Add document #2780

mxinden · 2022-07-28T07:41:10Z

Description

Add document outlining a set of coding guidelines followed and to be followed across the rust-libp2p code base.

My goal is not to impose these guidelines onto anyone, nor to be dogmatic about them. Instead I would like to discuss their value, eventually establishing a shared mindset, helping newcomers getting started faster.

For now, this is a draft only. Lots of the wording still need polishing. Suggestions / feedback / disagreement is welcome anytime.

Links to any relevant issues

This might be a helpful resource for discussions like #2622 (comment) in the future.

Open Questions

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

Add document outlining a set of coding guidelines followed and to be followed across the rust-libp2p code base.

thomaseizinger

Excellent document, lots of good knowledge here :)

Left some comments and ignored typos for now, we can deal with those once we settle on the content.

thomaseizinger · 2022-07-28T09:17:33Z

docs/coding-guidelines.md

+If you sqint, rust-libp2p is just a big hierarchy of [state
+machines](https://en.wikipedia.org/wiki/Finite-state_machine) where parents pass


Personally, I am a fan of using a "one-line per sentence" rule for version-controlled markdown documents.

Git works on a line-by-line basis and sentences in natural language tend to change as a whole too. Using one sentence per line makes it really easy to use GitHub's suggestions to collaborate on documents like this and diffs are more meaningful too.

Just an idea :)

But I have to admit I am also in the "use an editor with word wrap enabled"-camp :D

@mxinden Any comment on this? We don't have to resolve it here. Should I open a separate discussion?

Like this, correct @thomaseizinger? mxinden@ecf86c8

With GitHubs diff highlighting showing what within a line has changed, I don't think there is a great benefit for it. As a downside, I think it adds complexity, e.g. having to explain this to newcomers.

Just an idea :)

I don't feel strongly about it. It seems like you don't feel strongly about it either. Thus I will proceed without the change.

That said, feel free to propose a new pull request with the commit above.

But I have to admit I am also in the "use an editor with word wrap enabled"-camp :D

Same here. I even stopped truncating my emails to 80 characters 🤯

If I get to choose, I'd pick one sentence per line.

We don't have to enforce it though (I am yet to find a tool that reliably does).

Are you happy with doing it on a personal preference basis?

Are you happy with doing it on a personal preference basis?

For markdown files on rust-libp2p? Yes, that is fine by me.

docs/coding-guidelines.md

thomaseizinger · 2022-07-28T09:20:46Z

docs/coding-guidelines.md

+    match self.child_2.poll(cx) {
+      // As above.
+    }


I think it is important to mention that using continue in here will go back to child1 which could make progress again.

There is a subtle difference between looping over the entire thing and exhausting each child until it returns Pending. We want the former, not the latter.

Agreed, though not sure how to put it into words.

There is this line already:

// or `continue`, thus polling `child_1` again. `child_1` can potentially make more progress: continue

Could you make a suggestion @thomaseizinger ?

I'd suggest to just fill in the match here too and make a comment next to the continue of the Poll::Pending branch.

I'd suggest to just fill in the match here too

As in the same as the above? What would be the benefit of that? Isn't that just duplicating the same comments once more?

I wouldn't duplicate the entire explanation but perhaps have a complete example of the control flow and make one comment at the bottom that highlights that we always continue to go back to the top. Even though it should be clear from the written text, I find that seeing the entire control flow is better.

Maybe also build in the case where the event from child2 is dispatched to child1?

I added ed4d633. Let me know what you think.

docs/coding-guidelines.md

thomaseizinger · 2022-07-28T09:23:29Z

docs/coding-guidelines.md

+When using channels (e.g. `futures::channel::mpsc` or `std::sync::mpsc`)
+always use the bounded variant, never use the unbounded variant. When using a


We should use clippy's mechanism of banning these functions!

https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_methods

Good idea. Though I am failing to have clippy pick up my Clippy.toml when I save it in the root of the repository.

disallowed-methods = [ { path = "futures::sync::mpsc::channel", reason = "does not enforce backpressure" }, ]

@thomaseizinger does this work for you?

It reliably works for me after a cargo clean.

Will be introduced with #2823.

melekes

👍

melekes · 2022-08-01T07:01:05Z

docs/coding-guidelines.md

+Using hierarchical state machines is a deliberate choice throughout the
+rust-libp2p code base. It makes reasoning about control and data flow easy and
+works well with Rust's `Future` model. The archetecture pattern of
+hierarchical state machines should be used wherever possible.


Are there downsides to this design? If so, should we list them?

In my experience, the main downsides are:

It feels more verbose but I think it is not actually in reality when you would compare two solutions side by side.

The mix of control flow (loop, return, break, continue) in poll functions together with the asynchronous and thus decoupled communication via events can be very hard to understand.

Both are a form of complexity that we are trading for correctness and performance which seems typical in Rust land.

I documented the above in 0ef322a. @thomaseizinger I really liked your wording which I mostly adopted. Hope you don't mind.

@melekes in case you can think of other downsides, please comment and let's get them included in the discussion and the document.

melekes · 2022-08-01T07:01:59Z

docs/coding-guidelines.md

+
+      // The child did not make progress. It has registered the waker for a
+      // later wake up. Proceed with the other childs.
+      Poll::Ready(_) => {


Suggested change

Poll::Ready(_) => {

Poll::Pending => {

?

🙏 2b40afa

thomaseizinger · 2022-08-03T08:45:48Z

This blogpost might also go onto a further reading list: https://sans-io.readthedocs.io/how-to-sans-io.html

mxinden · 2022-08-04T07:14:36Z

This blogpost might also go onto a further reading list: https://sans-io.readthedocs.io/how-to-sans-io.html

I am a great fan of this coding style. It makes testing so easy.

That said, I don't think we apply this coding style anywhere within the rust-libp2p code-base. E.g. a Transport like libp2p-dns wrapps an I/O source, multistream-select wraps an I/O source, a ConnectionHandler has direct access to an I/O source. Thus I am having difficulties relating it to these coding guidelines.

…oding-guidelines

thomaseizinger · 2022-08-04T11:25:44Z

This blogpost might also go onto a further reading list: sans-io.readthedocs.io/how-to-sans-io.html

I am a great fan of this coding style. It makes testing so easy.

That said, I don't think we apply this coding style anywhere within the rust-libp2p code-base. E.g. a Transport like libp2p-dns wrapps an I/O source, multistream-select wraps an I/O source, a ConnectionHandler has direct access to an I/O source. Thus I am having difficulties relating it to these coding guidelines.

Well, it does apply to some degree. NetworkBehaviour is basically a sans-IO state machine right? Swarm is passing events between the network and the behaviour with the minor exception that Substream has a boxed muxer inside that directly connects to the IO source. But given that we don't follow it fully, it is maybe not the best blog post to reference.

…oding-guidelines

thomaseizinger · 2022-08-16T07:58:26Z

docs/coding-guidelines.md

+# Coding Guidelines
+
+<!-- markdown-toc start - Don't edit this section. Run M-x markdown-toc-refresh-toc -->
+**Table of Contents**


GitHub Markdown has a built-in ToC:

It is a bit hidden so I don't know we want to rely on people using it if they want a ToC.

Neat. Though as you say, quite hidden. I suggest we keep the additional ToC, though that is not a strong opinion.

thomaseizinger · 2022-08-16T08:01:35Z

docs/coding-guidelines.md

+machines](https://en.wikipedia.org/wiki/Finite-state_machine) where parents pass
+events down to their children and children pass events up to their parents.
+
+![Architecture](architecture.svg)


Instead of embedding the file, you could reference it planttext.com: https://www.planttext.com/api/plantuml/svg/SoWkIImgAStDuL80Wk3onA9S1PiQNLs5eFpy4gVKZCIopFpI8dH9v_oylDJaaipyl83yV0oj8KM9USK5-KKbO0aKWo0A0ZBpqb7DngB8CoKrhoGphPAWGk_4bDJSdCn4RAwrKYZ8pydHqCIY5qKArKEeSd3le0gZU09L2iFfgD8O3aP8EwJcfG0Z0m00

The same token also works as an "edit" link: https://www.planttext.com/?text=SoWkIImgAStDuL80Wk3onA9S1PiQNLs5eFpy4gVKZCIopFpI8dH9v_oylDJaaipyl83yV0oj8KM9USK5-KKbO0aKWo0A0ZBpqb7DngB8CoKrhoGphPAWGk_4bDJSdCn4RAwrKYZ8pydHqCIY5qKArKEeSd3le0gZU09L2iFfgD8O3aP8EwJcfG0Z0m00

I am not familiar with planttext. Is it a service we can rely on? What if they prune their database?

I've been using it for years without issues.

There is no database, I am pretty sure the diagram source code is embedded in the URL in some form of encoding.

That makes sense. I would argue though that there is benefit in being able to access the diagram and thus read the document without internet access.

docs/coding-guidelines.md

thomaseizinger · 2022-08-16T08:07:59Z

docs/coding-guidelines.md

+If you sqint, rust-libp2p is just a big hierarchy of [state
+machines](https://en.wikipedia.org/wiki/Finite-state_machine) where parents pass


@mxinden Any comment on this? We don't have to resolve it here. Should I open a separate discussion?

Co-authored-by: Thomas Eizinger <[email protected]>

When using channels (e.g. `futures::channel::mpsc` or `std::sync::mpsc`) always use the bounded variant, never use the unbounded variant. When using a bounded channel, a slow consumer eventually slows down a fast producer once the channel bound is reached, ideally granting the slow consumer more system resources e.g. CPU time, keeping queues small and thus latencies low. When using an unbounded channel a fast producer continues being a fast producer, growing the channel buffer indefinitely, increasing latency until the illusion of unboundedness breaks and the system runs out of memory. One may use an unbounded channel if one enforces backpressure through an out-of-band mechanism, e.g. the consumer granting the producer send-tokens through a side-channel. See also libp2p#2780 (comment)

docs/coding-guidelines: Add document

b6157e0

Add document outlining a set of coding guidelines followed and to be followed across the rust-libp2p code base.

thomaseizinger reviewed Jul 28, 2022

View reviewed changes

thomaseizinger mentioned this pull request Jul 29, 2022

feat: Add WebRTC transport #2622

Merged

4 tasks

docs/coding-guidelines: Document benefit controlling poll order

7714a30

melekes reviewed Aug 1, 2022

View reviewed changes

docs/coding-guidelines: Document favoring iteration over recursion

3b07e71

mxinden added 2 commits August 4, 2022 16:01

docs/coding-guidelines: Fix typo Ready to Pending

2b40afa

docs/coding-guidelines: Loop on work prioritization

995fd8c

Merge branch 'master' of https://github.com/libp2p/rust-libp2p into c…

cc89ff5

…oding-guidelines

elenaf9 mentioned this pull request Aug 5, 2022

*: Replace Future impl with poll_next method libp2p/if-watch#23

Merged

mxinden mentioned this pull request Aug 10, 2022

CONTRIBUTING.md #2808

Closed

mxinden added 5 commits August 13, 2022 09:59

docs/coding-guidelines: Fill out entire control flow on example

ed4d633

Merge branch 'master' of https://github.com/libp2p/rust-libp2p into c…

8a50031

…oding-guidelines

docs/coding-guidelines: Document hierarchical state machine downsides

0ef322a

docs/coding-guidelines: Reword async/await

412f03e

docs/coding-guidelines: Replace ASCII with svg

87c3bb8

mxinden marked this pull request as ready for review August 13, 2022 01:23

mxinden requested a review from thomaseizinger August 13, 2022 01:24

docs/coding-guidelines: Fix typo

69ea476

thomaseizinger approved these changes Aug 16, 2022

View reviewed changes

Update docs/coding-guidelines.md

eb0b75d

Co-authored-by: Thomas Eizinger <[email protected]>

mxinden mentioned this pull request Aug 17, 2022

clippy.toml: Create config and disallow unbounded channels #2823

Merged

4 tasks

Merge branch 'master' into coding-guidelines

f36be85

mxinden merged commit 475289c into libp2p:master Aug 17, 2022

mxinden mentioned this pull request Aug 24, 2022

docs/coding-guidelines: Document limit on number of tasks #2839

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs/coding-guidelines: Add document #2780

docs/coding-guidelines: Add document #2780

mxinden commented Jul 28, 2022

thomaseizinger left a comment

thomaseizinger Jul 28, 2022

thomaseizinger Aug 4, 2022

thomaseizinger Aug 16, 2022

mxinden Aug 17, 2022

thomaseizinger Aug 17, 2022

mxinden Aug 20, 2022

thomaseizinger Jul 28, 2022

mxinden Aug 1, 2022

thomaseizinger Aug 1, 2022

mxinden Aug 4, 2022

thomaseizinger Aug 4, 2022

mxinden Aug 13, 2022

thomaseizinger Jul 28, 2022

mxinden Aug 4, 2022

thomaseizinger Aug 4, 2022

mxinden Aug 17, 2022

melekes left a comment

melekes Aug 1, 2022

thomaseizinger Aug 1, 2022 •

edited

Loading

mxinden Aug 13, 2022

melekes Aug 1, 2022

mxinden Aug 4, 2022

thomaseizinger commented Aug 3, 2022

mxinden commented Aug 4, 2022

thomaseizinger commented Aug 4, 2022

thomaseizinger Aug 16, 2022

mxinden Aug 17, 2022

thomaseizinger Aug 16, 2022

mxinden Aug 17, 2022

thomaseizinger Aug 17, 2022

mxinden Aug 17, 2022

thomaseizinger Aug 16, 2022

		If you sqint, rust-libp2p is just a big hierarchy of [state
		machines](https://en.wikipedia.org/wiki/Finite-state_machine) where parents pass

		When using channels (e.g. `futures::channel::mpsc` or `std::sync::mpsc`)
		always use the bounded variant, never use the unbounded variant. When using a

docs/coding-guidelines: Add document #2780

docs/coding-guidelines: Add document #2780

Conversation

mxinden commented Jul 28, 2022

Description

Links to any relevant issues

Open Questions

Change checklist

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melekes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger Aug 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger commented Aug 3, 2022

mxinden commented Aug 4, 2022

thomaseizinger commented Aug 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger Aug 1, 2022 •

edited

Loading