-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Coroutinize http client code #2429
base: master
Are you sure you want to change the base?
Changes from 2 commits
797dc34
32365d4
e8df44b
dd51c2c
ad9833d
9b46ea5
86d6a8b
14e3efc
c5a63be
9b473bf
ffacf24
cb1b4b6
bd36cf3
bdfdf16
2d1550e
215e024
88de135
b10c8b6
685f6f2
200ea10
4250bca
688ac04
3e22a25
02df7b8
4dfb090
539fa2a
3231966
67134fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ module; | |
#endif | ||
|
||
#include <concepts> | ||
#include <coroutine> | ||
#include <memory> | ||
#include <optional> | ||
#include <stdexcept> | ||
|
@@ -77,38 +78,28 @@ connection::connection(connected_socket&& fd, internal::client_ref cr) | |
future<> connection::write_body(const request& req) { | ||
if (req.body_writer) { | ||
if (req.content_length != 0) { | ||
return req.body_writer(internal::make_http_content_length_output_stream(_write_buf, req.content_length, req._bytes_written)).then([&req] { | ||
if (req.content_length == req._bytes_written) { | ||
return make_ready_future<>(); | ||
} else { | ||
return make_exception_future<>(std::runtime_error(format("partial request body write, need {} sent {}", req.content_length, req._bytes_written))); | ||
} | ||
}); | ||
co_await req.body_writer(internal::make_http_content_length_output_stream(_write_buf, req.content_length, req._bytes_written)); | ||
if (req.content_length != req._bytes_written) { | ||
throw std::runtime_error(format("partial request body write, need {} sent {}", req.content_length, req._bytes_written)); | ||
} | ||
} else { | ||
co_await req.body_writer(internal::make_http_chunked_output_stream(_write_buf)); | ||
co_await _write_buf.write("0\r\n\r\n"); | ||
} | ||
return req.body_writer(internal::make_http_chunked_output_stream(_write_buf)).then([this] { | ||
return _write_buf.write("0\r\n\r\n"); | ||
}); | ||
} else if (!req.content.empty()) { | ||
return _write_buf.write(req.content); | ||
} else { | ||
return make_ready_future<>(); | ||
co_await _write_buf.write(req.content); | ||
} | ||
} | ||
|
||
future<connection::reply_ptr> connection::maybe_wait_for_continue(const request& req) { | ||
if (req.get_header("Expect") == "") { | ||
return make_ready_future<reply_ptr>(nullptr); | ||
if (req.get_header("Expect") != "") { | ||
co_await _write_buf.flush(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The common case is, that Expect will be empty, no? so we add an allocation here. Please measure the performance of the series. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The allocation is unavoidable here anyway, at the end this method does
ok There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
With #2442 , the test makes 1M requests in
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please report allocs/op There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
reply_ptr rep = co_await recv_reply(); | ||
if (rep->_status != reply::status_type::continue_) { | ||
co_return rep; | ||
} | ||
} | ||
|
||
return _write_buf.flush().then([this] { | ||
return recv_reply().then([] (reply_ptr rep) { | ||
if (rep->_status == reply::status_type::continue_) { | ||
return make_ready_future<reply_ptr>(nullptr); | ||
} else { | ||
return make_ready_future<reply_ptr>(std::move(rep)); | ||
} | ||
}); | ||
}); | ||
co_return nullptr; | ||
} | ||
|
||
void connection::setup_request(request& req) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this add an allocation in the common case that the buffer isn't full?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but all users we have use lambda body writer and don't step on this branch. But lambda body writer also adds allocation here