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] async and .await chapter #3909

Draft
wants to merge 191 commits into
base: main
Choose a base branch
from
Draft

[WIP] async and .await chapter #3909

wants to merge 191 commits into from

Conversation

carols10cents
Copy link
Member

@carols10cents carols10cents commented Apr 30, 2024

A working draft of a new chapter on async and await! If you have been invited to give comments, please do so! (If you’re not someone we have actively invited, we kindly ask you to hold off: although we are working in public, we cannot absorb too much feedback at once. Thanks!)

View the rendered version of this PR at https://rust-lang.github.io/book/ch17-00-async-await.html.

Note

This branch does not include changes to the listings for the chapters after this new chapter. As @carols10cents put it:

😬 GitHub is straight up not having a good time rendering the full diff of #3908 with all the listing changes 😱

Additionally, we will need to make some other small changes to other chapters (and possibly some significant ones to the “final project” chapter) to account for having this new material in place. Those are all also out of scope for this PR!

We will primarily just be re-exporting this, though we may also have a
couple cases where we choose to implement something small around it.
1. Introduce an integration tests crate. Structure it the way people
   *should* for large crates (although this is not that) and document
   why, including linking to a relevant post.
2. Add a basic integration test that verifies the re-export works as it
   should. (This is not exactly rocket science, but we want to make sure
   these things don’t just stop working on accident.)

An open question that remains here: do we want some structure to the
crate beyond the top level re-exports? My inclination at this moment is:
no, because we don’t have any *motivation* for that, and naming things
is difficult. (We cannot do `trpl::async`, for example, because `async`
is a keyword!)
- 17.00: Introduction to the conceptual machinery. This is very nascent
  but has some decent bones.
- 17.01: Trying to get at the foundations for tasks, laziness, etc.;
  this is *especially* incomplete.
- 17.02: Just a paragraph I do not want to lose, which I think *will*
  be useful… eventually.
Add a fair bit more material about the `futures` executor and why we
might prefer to use something else. With that motivation in place, have
the readers add our `trpl` crate. (We can of course rename that crate,
but it does the job for now.) Use it to get the equivalent of
`#[tokio::main]` equivalent and incorporate it into an example.
Copy link
Member Author

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

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

Some context on these comments: I know this is an early draft, but I have pointed out some things that appear small but I think are important for us to figure out while working, like figuring out what terminology we should standardize on and where we should define which terms. Please feel free to tell me "yeah yeah I agree but don't want to think about that right now" :)

src/ch17-00-async-await.md Outdated Show resolved Hide resolved
src/ch17-00-async-await.md Outdated Show resolved Hide resolved
src/ch17-00-async-await.md Outdated Show resolved Hide resolved
src/ch17-00-async-await.md Outdated Show resolved Hide resolved
src/ch17-00-async-await.md Outdated Show resolved Hide resolved
src/ch17-01-tasks.md Outdated Show resolved Hide resolved
src/ch17-01-tasks.md Outdated Show resolved Hide resolved
src/ch17-01-tasks.md Outdated Show resolved Hide resolved
src/ch17-01-tasks.md Outdated Show resolved Hide resolved
src/ch17-01-tasks.md Outdated Show resolved Hide resolved
@chriskrycho
Copy link
Contributor

things that appear small but I think are important for us to figure out while working, like figuring out what terminology we should standardize on and where we should define which terms.

1,000% on board with this!

Also very on board with working on this separate branch. I will likely keep the other around for reference (and we may even be able to merge it when all is said and done, who knows), but it will be a lot easier to deal with the mechanical bits separately from the prose bits, and we really don’t want to land any of that till we are actually ready to land the chapter. 😅

I have replied to most of the comments here, and I have read all of them; if I haven’t yet replied to one it is just because I am still mulling on it!

@chriskrycho chriskrycho force-pushed the only-new-async branch 2 times, most recently from f3fc42f to 87a015f Compare May 10, 2024 14:46
mdBook does not currently have particularly good support for “external”
crates. To make the test suite work correctly with `trpl`, we must first
build `trpl` itself (`mdbook` will not do it), and then explicitly pass
its `deps` path as a library search path for `mdbook test`. That will
make sure all the crates can be resolved when running the tests.


.github/workflows/main.yml
- Update the contents of the code in the chapter so it is correct, and
  update the text to match. Given this is about `Future`, this may also
  warrant moving/simplifying that whole chunk of code, too.

- Update the listing to use `extern crate` since it does not otherwise
  work correctly with `mdbook test`. Alas.
- Update all the listings in the chapter to use `extern crate` since
  `mdbook test` does not understand Rust 2018. Alas.
- Ignore two of the listings because they never stop.
- Update all the listings in the chapter to use `extern crate` since
  `mdbook test` does not understand Rust 2018. Alas.
- Fix one listing which had gotten out of sync somewhere along the way.
  (This also fixes a comment flagged up by a reviewer!)
- Update all the listings in the chapter to use `extern crate` since
 `mdbook test` does not understand Rust 2018. Alas.
- Ignore a listing which has a missing body apurpose.
- Rewrite the `StreamExt` definition to be more correct, and extract it
  to a standalone “no-listing listing” so we can make sure its
  definition at least type checks.
- Update all the listings in the chapter to use `extern crate` since
 `mdbook test` does not understand Rust 2018. Alas.
- Ignore the listings (inline or otherwise!) which are *intended* not to
  compile, so mdbook does not try to test them.
- Clarify some of the text around the *actual* Tokio definition of the
  `StreamExt::next` method.
@chriskrycho chriskrycho mentioned this pull request Jul 17, 2024
4 tasks
Note: this does *not* include all fixes for the text, only for the links
themselves. For the text, we will also need to search for references to
chapters 17-20. This catches a few of those along the way, but there are
no doubt others.
Copy link
Member Author

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

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

Submitting notes through the end of 17-01

src/ch17-00-async-await.md Outdated Show resolved Hide resolved
src/ch17-00-async-await.md Outdated Show resolved Hide resolved
src/ch17-00-async-await.md Show resolved Hide resolved
src/ch17-01-futures-and-syntax.md Show resolved Hide resolved
src/ch17-01-futures-and-syntax.md Show resolved Hide resolved
src/ch17-01-futures-and-syntax.md Outdated Show resolved Hide resolved
src/ch17-01-futures-and-syntax.md Show resolved Hide resolved
src/ch17-01-futures-and-syntax.md Show resolved Hide resolved
src/ch17-01-futures-and-syntax.md Show resolved Hide resolved
src/ch17-01-futures-and-syntax.md Show resolved Hide resolved
Copy link
Member Author

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

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

Almost to the end of 17-03

src/ch17-02-concurrency-with-async.md Show resolved Hide resolved
src/ch17-02-concurrency-with-async.md Show resolved Hide resolved
src/ch17-02-concurrency-with-async.md Show resolved Hide resolved
src/ch17-02-concurrency-with-async.md Show resolved Hide resolved
src/ch17-02-concurrency-with-async.md Show resolved Hide resolved
src/ch17-03-more-futures.md Show resolved Hide resolved

</Listing>

This is definitely a nice improvement over needing to swap between `join` and
Copy link
Member Author

@carols10cents carols10cents Jul 23, 2024

Choose a reason for hiding this comment

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

I'm not sure we need to show join and join3-- the reader has been using println! and (should) know that macros are for taking a variable number of arguments. Any particular reason not to start out using the macro? Then at this point, talk about the common pattern?

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh! I don’t think I had ever actually framed it as “macros are for taking a variable number of arguments” before reading this comment from you! Interesting. I am definitely open to revisiting the order. I think I defaulted away from using join! because of how bad a taste tokio::select! left in my mouth, but futures::join! is much, much less magical, so I don’t have any objection to doing that. Will leave this one for a further pass, though, after getting through all the other stuff, since it will require some rejiggering of listings etc.

src/ch17-03-more-futures.md Show resolved Hide resolved
src/ch17-03-more-futures.md Show resolved Hide resolved
Comment on lines +306 to +312
`Pin` builds on that to give us the exact guarantee we need. When we *pin* a
value by wrapping a pointer to it in `Pin`, it can no longer move. Thus, if you
have `Pin<Box<SomeType>>`, you actually pin the `SomeType` value, *not* the
`Box` pointer. In fact, the pinned box pointer can move around freely. Remember:
we care about making sure the data ultimately being referenced stays in its
place. If a pointer moves around, but the data it points to is in the same
place, there is no problem.
Copy link
Member Author

Choose a reason for hiding this comment

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

This could use a diagram, along the lines of some of the ones in chapter 4

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, very good call; I will make a diagram for this! (Note also that this whole section is now part of a new 17.05, which pulls together all the walk-throughs of the relevant traits.)

chriskrycho and others added 3 commits July 23, 2024 13:32
Co-authored-by: Tim McNamara <paperless@timmcnamara.co.nz>
Co-authored-by: James Munns <james@onevariable.com>
Co-authored-by: Carol (Nichols || Goulding) <carol.nichols@gmail.com>
Co-authored-by: Will Crichton <crichton.will@gmail.com>
Co-authored-by: Tim McNamara <paperless@timmcnamara.co.nz>
Copy link
Member Author

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

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

Partway through 17-05

So far so good: if we get anything wrong about the ownership or references in a
given async block, the borrow checker will tell us. When we want to move around
the future that corresponds to that block—like moving it into a `Vec` to pass to
`join_all`—things get trickier.
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 think around this spot it would be awesome if we had an example of, say, trying to use a reference in an async block, show the error message, and show how to fix it-- I get that sort of error message alllll the time when i'm switching between sync and async code because it looks like i'm just within a block or something and it should be totally fine to use a reference and then error message and then i have to remember to clone the arc or whatever.

Copy link
Contributor

Choose a reason for hiding this comment

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

@carols10cents I am going to leave this specific comment open but without making a specific change related to it, because I think the overall suggestion here is a good one but I need to mull on how it works with, and where it belongs in, the chapter after the restructuring I did. It does not flow well with this paragraph where it landed in the new section dedicated to discussing the traits, which is (in its current incarnation, anyway) much more abstract. But I think that it probably needs exactly this kind of thing to help it not be too abstract. I am going to thus table that for the moment and leave this as a note to return to after hitting the rest of this PR’s comments and reworking the opening example.

src/ch17-03-more-futures.md Show resolved Hide resolved
src/ch17-04-more-ways-of-combining-futures.md Show resolved Hide resolved
src/ch17-04-more-ways-of-combining-futures.md Show resolved Hide resolved
src/ch17-04-more-ways-of-combining-futures.md Show resolved Hide resolved
src/ch17-04-more-ways-of-combining-futures.md Show resolved Hide resolved
src/ch17-04-more-ways-of-combining-futures.md Show resolved Hide resolved
src/ch17-04-more-ways-of-combining-futures.md Show resolved Hide resolved
src/ch17-04-more-ways-of-combining-futures.md Show resolved Hide resolved
src/ch17-05-streams.md Show resolved Hide resolved
Use the original transitional paragraph and structure, adding to it
instead of rewriting it entirely.

HT @timClicks (Tim McNamara <paperless@timmcnamara.co.nz>) for pointing
out how my rephrasing here made it worse!
- Drop the history lesson and comparisons to other approaches. Focus on
  what async gives us instead.
- Simplify and clarify the 
- Talk about “the network” instead of “network sockets” and simplify the
  example code to match.
- Swap the ordering to match the order in the text (concurrent is first,
  at least for now).
- Make them go left-to-right instead of top-to-bottom for compactness in
  the text.
- Fix rendering issues resulting from the VS Code extension doing things
  that normal `dot` does not, for who knows what reasons.
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.

5 participants