docs: show a coroutine-based implementation of the echo server in the tutorial #2173
Conversation
doc/tutorial.md
Outdated
| @@ -1778,6 +1778,40 @@ It is often a mistake to silently ignore an exception, so if the future we're ig | |||
|
|
|||
| The ```handle_connection()``` function itself is straightforward --- it repeatedly calls ```read()``` read on the input stream, to receive a ```temporary_buffer``` with some data, and then moves this temporary buffer into a ```write()``` call on the output stream. The buffer will eventually be freed, automatically, when the ```write()``` is done with it. When ```read()``` eventually returns an empty buffer signifying the end of input, we stop ```repeat```'s iteration by returning a ```stop_iteration::yes```. | |||
|
|
|||
| Re-written using C++20's coroutines, the above becomes this: | |||
|
|
|||
There was a problem hiding this comment.
IMO it should replace the original. We encourage coroutines now (except in one case - when the function usually resolves with a ready future and it is very performance sensitive). The original is very outdated, no one should use keep_doing().
There was a problem hiding this comment.
I think eventually we need to go over the tutorial and reorganize/rewrite it in a way that gives more explanations and examples using coroutines - and only later in a separate section introduces the finer points of continuations, how to use them, when to use them, and so on. In the continuations section we'll need some examples, including of keep_doing, but it doesn't need to be this specific server.
There was a problem hiding this comment.
Sounds great! Shall I remove the continuation-based implementation of the echo server and adjust the explanation to match the new coroutines-based one, or would you prefer to do this yourselves?
There was a problem hiding this comment.
Given the following goals:
- A: Remove the continuation-based version of the echo server and adjust the explanation to match the coroutine-based version.
- B: Convert the whole tutorial from continuations to coroutines.
I guess we have the following options for this PR here:
- Discard this PR (close unmerged).
- Merge it as it is (keep A and B for later)
- I do A in this PR.
(Doing B currently feels way above my understanding/skills.)
| auto listener = seastar::listen(seastar::make_ipv4_address({1234}), lo); | ||
| while (true) { | ||
| auto res = co_await listener.accept(); | ||
| (void) handle_connection(std::move(res.connection)); |
There was a problem hiding this comment.
Ignoring futures is bad practice in a real server it will lead to running out of memory, or to problems during shutdown. I'll accept it since it's so in the original, but at least add comments about it.
There was a problem hiding this comment.
Thanks for the remark. Just added the comment. ✔️
doc/tutorial.md
Outdated
| auto in = s.input(); | ||
| while (true) { | ||
| auto buf = co_await in.read(); | ||
| if (buf) { |
There was a problem hiding this comment.
Nicer: while (auto buf = co_await in.read()) {, makes the exist condition clear.
There was a problem hiding this comment.
Oh, what a great idea! ❤️
I just committed this change. ✔️
|
/cc @nyh |
|
If you have any suggestions on what I can do to help move this PR forward, please let me know. In case it's currently not wanted, and you'd prefer to close this unmerged, that's also ok for me. |
Replace the continuations based example with the coroutine. There's no reason to show both. |
|
Thanks! I just did that in this commit which also adjusts the explanation text accordingly. ✔️ |
Hi,
this pull request is meant mainly as a proposal. For me, it was eye-opening to convert the continuation-based implementation of the TCP-echo server to coroutines. So I thought seeing this might help others too. But I don't know if it makes sense to have it in the tutorial. So, if you don't see fit, I'd fully understand if you simply close this PR without merging.☺️