Add an HTTP `ResponseFuture` struct #1008

zeylahellyer · 2021-07-03T18:43:41Z

We currently take owned types, such as Vecs and Strings. This is due to the lifetimes around requests, their inner polled futures, and the caller are difficult to work around.

The way requests currently work is that they implement std::future::Future and "start" a future when initially polled for the first time. They then store this future internally in a field like so:

pub struct SomeRequest {
    fut: Option<Pending<'a, Message>>,
    // more fields...
}

This requires a lot of boilerplate on our part: we have a macro implementing Future on every request with more or less unique implementations and need fields for the inner future.

For users, this design comes at a cost of performance and clarity. Users need to give us owned arguments. It is also impossible for users to determine whether a request is in flight using the type system due to that information being hidden. We need to clone arguments if we want to avoid panics from users accidentally double polling a request after completion, when it may not be clear that a request has been used due to the request not being consumed.

Instead, we can turn the inner request functions that "start" a future into ones that consume and execute the request and then return a future resolving to a response directly to the user. The new response::ResponseFuture implements Poll and will begin the execution of a request - if a failure did not occur during construction - while polling it to completion, providing the resulting response::Response.

Internally the implementation of the CreateMessage::exec function looks like this:

pub fn exec(self) -> ResponseFuture<Message> {
    let request = Request::from_route(Route::GetMessage {
        channel_id: self.channel_id.0,
        message_id: self.message_id.0,
    });

    self.http.request(request)
}

The usage of executing the request and printing the message contents looks like this:

let msg = client.message(channel_id, message_id).exec().await?;
let message = response.model().await?;

println!("content: {}", message.content);

This work does not include the optimization work that turns all of our owned parameters to borrowed ones and will be done by a separate PR.

~~Blocks on #923.~~ Merged.

We currently take owned heap allocated types, such as Vecs and Strings. This is due to the lifetimes around requests, their inner polled futures, and the caller are difficult to work around. The way requests currently work is that they implement `std::future::Future` and "start" a future when initially polled for the first time. They then store this future internally in a field like so: ```rust pub struct SomeRequest { fut: Option<Pending<'a, Message>>, // more fields... } ``` This requires a lot of boilerplate on our part: we have a macro implementing `Future` on every request with more or less unique implementations and need fields for the inner future. For users, this design comes at a cost of performance and clarity. Users need to give us owned arguments. It is also impossible for users to determine whether a request is in flight using the type system due to that information being hidden. We need to clone arguments if we want to avoid panics from users accidentally double polling a request after completion, when it may not be clear that a request has been used due to the request not being consumed. Instead, we can turn the inner request functions that "start" a future into ones that consume and execute the request and then return a future resolving to a response directly to the user. The new `response::ResponseFuture` implements `Poll` and will begin the execution of a request - if a failure did not occur during construction - while polling it to completion, providing the resulting `response::Response`. Internally the implementation of the `CreateMessage::exec` function looks like this: ```rust pub fn exec(self) -> ResponseFuture<Message> { let request = Request::from_route(Route::GetMessage { channel_id: self.channel_id.0, message_id: self.message_id.0, }); self.http.request(request) } ``` The usage of executing the request and printing the message contents looks like this: ```rust let msg = client.message(channel_id, message_id).exec().await?; let message = response.model().await?; println!("content: {}", message.content); ``` This work does not include the optimization work that turns all of our owned parameters to borrowed ones and will be done by a separate PR. Signed-off-by: Zeyla Hellyer <zeyla@hellyer.dev>

zeylahellyer · 2021-07-03T23:39:14Z

Rebased with the merging of #923.

http/src/request/application/update_command_permissions.rs

http/src/request/application/update_global_command.rs

http/src/request/application/delete_guild_command.rs

http/src/request/application/interaction_callback.rs

http/src/request/application/set_command_permissions.rs

http/src/request/application/set_global_commands.rs

http/src/request/application/set_guild_commands.rs

http/src/request/application/update_guild_command.rs

http/src/request/channel/reaction/create_reaction.rs

http/src/request/guild/member/search_guild_members.rs

Signed-off-by: Zeyla Hellyer <zeyla@hellyer.dev>

zeylahellyer · 2021-07-09T01:34:35Z

Addressed comments.

Concerns have been resolved or answered.

We currently take owned heap allocated types, such as Vecs and Strings. This is due to the lifetimes around requests, their inner polled futures, and the caller are difficult to work around. The way requests currently work is that they implement `std::future::Future` and "start" a future when initially polled for the first time. They then store this future internally in a field like so: ```rust pub struct SomeRequest { fut: Option<Pending<'a, Message>>, // more fields... } ``` This requires a lot of boilerplate on our part: we have a macro implementing `Future` on every request with more or less unique implementations and need fields for the inner future. For users, this design comes at a cost of performance and clarity. Users need to give us owned arguments. It is also impossible for users to determine whether a request is in flight using the type system due to that information being hidden. We need to clone arguments if we want to avoid panics from users accidentally double polling a request after completion, when it may not be clear that a request has been used due to the request not being consumed. Instead, we can turn the inner request functions that "start" a future into ones that consume and execute the request and then return a future resolving to a response directly to the user. The new `response::ResponseFuture` implements `Poll` and will begin the execution of a request - if a failure did not occur during construction - while polling it to completion, providing the resulting `response::Response`. Internally the implementation of the `CreateMessage::exec` function looks like this: ```rust pub fn exec(self) -> ResponseFuture<Message> { let request = Request::from_route(Route::GetMessage { channel_id: self.channel_id.0, message_id: self.message_id.0, }); self.http.request(request) } ``` The usage of executing the request and printing the message contents looks like this: ```rust let msg = client.message(channel_id, message_id).exec().await?; let message = response.model().await?; println!("content: {}", message.content); ``` This work does not include the optimization work that turns all of our owned parameters to borrowed ones and will be done by a separate PR. Signed-off-by: Zeyla Hellyer <zeyla@hellyer.dev>

@zeylahellyer

Enhancements Many functions have been made constant ([#1010] - [@zeylahellyer]). Changes There are significant changes to how users make HTTP requests. When users make a request, they must pass borrowed types instead of owned types. To execute the request, users must call `exec` on the request builder. Once the request has completed execution, users may use the `ResponseFuture` struct methods to access the status code of the request. To access a returned model, if there is one, users must call `model` on the response. A call to `Client::create_message` like this: ```rust client.create_message(ChannelId(1)) .content("some content")? .embed(Embed {})? .await?; ``` is now written like this: ```rust client.create_message(ChannelId(1)) .content(&"some conntent")? .embeds(&[&Embed {}])? .exec() .await? .model() .await?; ``` For more information on the motivation behind these changes, see the PR descriptions of [#923], [#1008], and [#1009]. These changes were authored by [@zeylahellyer]. Rename `ErrorCode::UnallowedWordsForPublicStage` variant to `UnallowedWords` ([#956] - [@7596ff]) `CreateGlobalCommand`, `CreateGuildCommand`, `SetGlobalCommands`, and `SetGuildCommands` now return command(s) ([#1037] - [@vilgotf]). A few spelling errors have been fixed by adding the `codespell` Action ([#1041] - [@Gelbpunkt]. [#923]: #923 [#956]: #956 [#1008]: #1008 [#1009]: #1009 [#1010]: #1010 [#1037]: #1037 [#1041]: #1041 [@7596ff]: https://github.com/7596ff [@Gelbpunkt]: https://github.com/Gelbpunkt [@vilgotf]: https://github.com/vilgotf [@zeylahellyer]: https://github.com/zeylahellyer

We currently take owned heap allocated types, such as Vecs and Strings. This is due to the lifetimes around requests, their inner polled futures, and the caller are difficult to work around. The way requests currently work is that they implement `std::future::Future` and "start" a future when initially polled for the first time. They then store this future internally in a field like so: ```rust pub struct SomeRequest { fut: Option<Pending<'a, Message>>, // more fields... } ``` This requires a lot of boilerplate on our part: we have a macro implementing `Future` on every request with more or less unique implementations and need fields for the inner future. For users, this design comes at a cost of performance and clarity. Users need to give us owned arguments. It is also impossible for users to determine whether a request is in flight using the type system due to that information being hidden. We need to clone arguments if we want to avoid panics from users accidentally double polling a request after completion, when it may not be clear that a request has been used due to the request not being consumed. Instead, we can turn the inner request functions that "start" a future into ones that consume and execute the request and then return a future resolving to a response directly to the user. The new `response::ResponseFuture` implements `Poll` and will begin the execution of a request - if a failure did not occur during construction - while polling it to completion, providing the resulting `response::Response`. Internally the implementation of the `CreateMessage::exec` function looks like this: ```rust pub fn exec(self) -> ResponseFuture<Message> { let request = Request::from_route(Route::GetMessage { channel_id: self.channel_id.0, message_id: self.message_id.0, }); self.http.request(request) } ``` The usage of executing the request and printing the message contents looks like this: ```rust let msg = client.message(channel_id, message_id).exec().await?; let message = response.model().await?; println!("content: {}", message.content); ``` This work does not include the optimization work that turns all of our owned parameters to borrowed ones and will be done by a separate PR. Signed-off-by: Zeyla Hellyer <zeyla@hellyer.dev>

zeylahellyer added t-feature Addition of a new feature c-http Affects the http crate m-breaking change Breaks the public API. labels Jul 3, 2021

zeylahellyer mentioned this pull request Jul 3, 2021

Take references in request parameters #1009

Merged

7596ff previously requested changes Jul 9, 2021

View reviewed changes

address comments

52b49f4

Signed-off-by: Zeyla Hellyer <zeyla@hellyer.dev>

zeylahellyer requested a review from 7596ff July 9, 2021 01:34

7596ff approved these changes Jul 9, 2021

View reviewed changes

BlackHoleFox approved these changes Jul 10, 2021

View reviewed changes

zeylahellyer merged commit 7f05145 into twilight-rs:next Jul 10, 2021

zeylahellyer deleted the feature/http/response-future branch July 10, 2021 22:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an HTTP `ResponseFuture` struct #1008

Add an HTTP `ResponseFuture` struct #1008

zeylahellyer commented Jul 3, 2021 •

edited

Loading

zeylahellyer commented Jul 3, 2021

zeylahellyer commented Jul 9, 2021

Add an HTTP ResponseFuture struct #1008

Add an HTTP ResponseFuture struct #1008

Conversation

zeylahellyer commented Jul 3, 2021 • edited Loading

zeylahellyer commented Jul 3, 2021

zeylahellyer commented Jul 9, 2021

Add an HTTP `ResponseFuture` struct #1008

Add an HTTP `ResponseFuture` struct #1008

zeylahellyer commented Jul 3, 2021 •

edited

Loading