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

AK+LibHTTP+LibTest: Introduce more bugs in asynchronous code #24310

Merged
merged 13 commits into from
Jun 13, 2024

Conversation

DanShaders
Copy link
Contributor

@DanShaders DanShaders commented May 13, 2024

You thought you mastered idiosyncrasies of AK::Stream? This PR gets you covered by adding a similar but completely unrelated way of doing datastreams.

You thought you figured out how to write event-driven asynchronous code in Serenity? This PR gets you covered by adding a similar but subtly different way for dealing with asynchronousness.

You thought you knew how and when lifetimes are ended? This PR gets you covered by complicating things so much you would start to question your C++ skills.

You thought you knew how to read backtraces in GDB? This PR gets you covered by converting backtraces into an unreadable mess going both in call-stack and reverse call-stack directions while interleaving actual calls with visual clutter.

You thought you knew what I would upstream first? This PR gets you covered by choosing a different subset of features to contribute.


As much as I would like to leave only the first 5 sentences in the PR description, I guess I have to properly explain some things here. So, I think coroutines are ready to be experimented on in tree. Thus, this PR implements AK::Coroutine, defines and gives an extremely detailed description of AK::Async{Input,Output,}Streams interfaces, and implements a very bare-bones fully asynchronous HTTP/1.1 client.

In order to prove that the architecture I chose for asynchronous streams is adequate, I've implemented a variety of streams, stream adapters, and stream wrappers, including but not limiting to AK::AsyncLexingAdapter for consuming data until a predefined delimiter, AK::AsyncInputStreamSlice for treating a prefix of the stream with some predefined length as a stream by itself, Test::AsyncMemory{Input,Output}Stream for treating memory locations as streams, HTTP::(anonymous namespace)::ChunkedBodyStream for treating a Transfer-Encoding: chunked-encoded HTTP response body as a stream, and (not present in this PR) AK::AsyncInputStreamLegacyTransform for treating an asynchronous stream transformed by legacy AK::Stream as asynchronous stream. All of these classes have a very simple and (after you spend dozens of hours using AK::Coroutine) intuitive implementations. Therefore, I don't expect the fundamental design of streams to change much anymore.

I know, however, that there are a few minor deficiencies in the current asynchronous framework. Namely, asynchronous input streams are basically asynchronous generators and it would be very inconvenient and error-prone to write more complicated transformations as hand-unrolled state machines (you can see what I mean by looking at ChunkedBodyStream and comment just above its enqueue_some). Additionally, current lockless design of AsyncOutputStream::write won't work for HTTP/2. However, these two problems can be dealt with later without significant changes to the code I add in this PR. Generated streams have been implemented. However, I now think that AsyncOutputStream design is just plain wrong, but again, it can be improved later.

Note that this PR doesn't contain asynchronous TCP socket as it requires a bit more work in general and Serenity lacks necessary runtime functions to support it. This doesn't mean that the code here can't be tested -- Test::Async{Input,Output}Stream combined using AK::AsyncStreamPair work well as a replacement for socket in tests (and actually allow proper unit testing of HTTP code).

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label May 13, 2024
@DanShaders DanShaders requested a review from ADKaster May 13, 2024 05:11
Copy link
Member

@ADKaster ADKaster left a comment

Choose a reason for hiding this comment

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

Basket of comments on the first two-thirds or so. I'll let Ali and timschumi comment on the rest. (esp the HTTP 1.1 thingy).

Userland/Libraries/LibCore/ThreadEventQueue.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibCore/ThreadEventQueue.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibCore/ThreadEventQueue.h Outdated Show resolved Hide resolved
Userland/Libraries/LibCore/ThreadEventQueue.cpp Outdated Show resolved Hide resolved
AK/Coroutine.h Show resolved Hide resolved
AK/AsyncStream.h Show resolved Hide resolved
AK/AsyncStream.h Show resolved Hide resolved
AK/AsyncStream.h Show resolved Hide resolved
Userland/Libraries/LibTest/AsyncTestCase.h Outdated Show resolved Hide resolved

namespace Detail {
template<typename T>
struct TryAwaiter {
Copy link
Member

Choose a reason for hiding this comment

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

Why is CO_TRY preferable to making ErrorOr awaitable? i.e. co_await TRY(...); or... no that doesn't work. co_await ErrorOr { TRY(...) }. Nope that doesn't work either.

I guess I've got mixed feelings about two things:

  • Why is this awaiter hidden in Detail, and not a handy dandy type we want to spell everywhere?
  • Why do we need another macro to wrap expressions in?

Copy link
Contributor Author

@DanShaders DanShaders May 13, 2024

Choose a reason for hiding this comment

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

Why is this awaiter hidden in Detail, and not a handy dandy type we want to spell everywhere?

Because of the same reason why we have TRY macro and not spell if (result.is_error()) { return result.release_error(); } everywhere like in go.

Why do we need another macro to wrap expressions in?

Because return inside TRY won't work in coroutines.

I could have done

#define CO_TRY(expression)                                                                           \
    ({                                                                                               \
        /* Ignore -Wshadow to allow nesting the macro. */                                            \
        AK_IGNORE_DIAGNOSTIC("-Wshadow",                                                             \
            auto&& _temporary_result = (expression));                                                \
        static_assert(!::AK::Detail::IsLvalueReference<decltype(_temporary_result.release_value())>, \
            "Do not return a reference from a fallible expression");                                 \
        if (_temporary_result.is_error()) [[unlikely]]                                               \
            co_return _temporary_result.release_error();                                             \
        _temporary_result.release_value();                                                           \
    })

but the awaiter I ended up with always does 1 less move.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upd: and also doesn't ICE GCC, who doesn't seem to like statement expressions in coroutines.

@awesomekling
Copy link
Contributor

What are some tangible benefits of this approach? You've spent several paragraphs joking about how this makes the code harder to understand and debug, but haven't said a word about what it makes better.

What are the throughput and latency improvements from these changes? Surely there is some measurable performance gain to justify the increase in complexity :)

@DanShaders DanShaders force-pushed the coro-new branch 9 times, most recently from 47515b0 to da04c3d Compare May 17, 2024 18:22
@DanShaders
Copy link
Contributor Author

DanShaders commented May 18, 2024

@awesomekling, actually I argue that it is easier to write correct asynchronous code with coroutines than without. For example, here is a streamable asynchronous implementation of deflate decompression. In total, it has 17 states across 3 different state machines, all of which were generated automatically. This would have been a total nightmare to write manually.

Why do we need asynchronous algorithms in the first place? Well, to not have threading monstrosity like CxBoog is introducing in #24313. We don't want to start a new thread for every compressed response needed to be streamed, do we?

Surely there is some measurable performance gain to justify the increase in complexity :)

In general, a synchronous implementation can always be made faster than an asynchronous one just because the former doesn't need to track its state. Coroutines selling point is not performance, it is interleaving. Despite that, since I designed an asynchronous datastream framework for the ground up in this PR, I made sure it can be made more performant than old streams (I'm also planning to gradually transition old streams to the new design later since I think it is much better). And, in fact, the inflate implementation I linked is 10% faster than our current synchronous implementation (despite them sharing a lot of code and the algorithm and the fact that I spend a grand total of 15 minutes optimizing it).

@DanShaders DanShaders force-pushed the coro-new branch 4 times, most recently from f9336a6 to e02d2cb Compare May 21, 2024 22:25
@dotnetCarpenter
Copy link

Well, at least this is interesting 🧐

@DanShaders DanShaders force-pushed the coro-new branch 2 times, most recently from 55edd69 to 1f21c5c Compare May 31, 2024 16:53
Otherwise it fails to resolve ambiguity between std::forward and
AK::forward when called with the std:: type.
@DanShaders
Copy link
Contributor Author

Changes in the last push:

  • Rebase on top of master
  • Fix compile error in Coroutine<void>::Coroutine(Coroutine<void>&&) noticed by Ali (and add a test for it)
  • Add a test for 0-length read of AsyncInputStream after EOF has been reached

@DanShaders DanShaders closed this Jun 3, 2024
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Jun 3, 2024
@DanShaders
Copy link
Contributor Author

Oops, not sure how this has happened

@DanShaders DanShaders reopened this Jun 4, 2024
@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Jun 4, 2024
Copy link
Member

@timschumi timschumi left a comment

Choose a reason for hiding this comment

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

Looks fine conceptually (starting with the Stream-specific parts as recommended), just have a few questions/requests for clarification.

Could use a bit of polish around the edges with regards to formatting (raw pointers, C-style casts, maybe sprinkle in some early returns where it makes sense logically).

AK/AsyncStream.h Outdated Show resolved Hide resolved
AK/AsyncStreamHelpers.h Outdated Show resolved Hide resolved
AK/AsyncStreamBuffer.h Outdated Show resolved Hide resolved
AK/AsyncStream.h Show resolved Hide resolved
@DanShaders DanShaders force-pushed the coro-new branch 3 times, most recently from afa1755 to 043b5ee Compare June 12, 2024 18:58
@DanShaders DanShaders changed the title AK+LibCore+LibHTTP+LibTest: Introduce more bugs in asynchronous code AK+LibHTTP+LibTest: Introduce more bugs in asynchronous code Jun 12, 2024
Otherwise it is impossible to use Badge as an argument of a coroutine.
They are useful for unit testing other asynchronous streams. They have
to be tested themselves in the first place though.
These were proven to be generally useful after writing some asynchronous
streams code.
This class allows to convert an asynchronous generator which generates
chunks of data into an AsyncInputStream. This is useful in practice
because the said generator often ends up looking very similar to the
underlying synchronous algorithm it describes.
This class encapsulates all the logic required for a buffer of
asynchronous stream.
We don't have asynchronous TCP socket implementation, so its usefulness
is a bit limited currently but we can still test it using memory
streams. Additionally, it serves as a temporary {show,test}case for the
asynchronous streams machinery.
This uses AK::{Generator,AsyncStreamTransform,AsyncStreamBuffer} added
in the previous commits.
@alimpfard
Copy link
Member

HTTP client possibly pending removal if left unused for a while, let's dew it.

@alimpfard alimpfard merged commit 7d1d0fe into SerenityOS:master Jun 13, 2024
11 checks passed
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Jun 13, 2024
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.

6 participants