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.

Sign up for GitHub

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

Jump to bottom

Custom backends support #405

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Custom backends support #405

wants to merge 17 commits into from

Conversation

ColonelThirtyTwo
Copy link
Contributor

Allows external crates to write their own Sender implementations and use them with Client.

This involved refactoring a decent amount of the crate, and probably has some backwards-incompatible changes. I didn't change any of the usual APIs though; most code that just does querying should still work.

The primary impetus of this change is getting this crate to work in WebAssembly. In that environment, requests must be made via the XMLHttpRequest API, and this crate must return JS promises. I have written such a backend, which serves as an example; code is here: https://gitlab.com/colonelthirtytwo/elastic-stdweb-rs

Major changes off the top of my head, see commits for more details:

  • Add TypedSender<TReqInner>, a subtrait of Sender that all backends should implement for all T: ReqInner. This handles deserializing the response from the backend once it is available (ex. using Result.map for SyncSender, Future::map for AsyncSender, and Promise::then for StdwebSender). The response type (an associated type of the trait) has to vary depending on the type being deserialized, hence why this is a separate trait from Sender. Once Generic Associated Types are stabilized, the trait can be merged into Sender.
  • Merge the different implementations on request types for SyncSender and AsyncSender into one that uses the TypedSender. Rust orphaning rules would forbid implementing Request<MySender> in external crates, so that approach would not scale.
  • Remove Sender field from SniffedNodes and replace it with an API that was not responsible for fetching the request, since how to fetch the request would change based on backend.
  • Because of the above, remove the NextParams trait and move its functionality to Sender::next_params.
  • Create a few traits and implement them on API structs to allow for more introspection to support TypedSender.
  • Made several sender-related things public so that crates implementing sender can use them.
  • The SyncSender, AsyncSender, and related code are now behind cargo features, allowing this crate to be built without them (default is to include both, for backwards compatibility). WASM needs this, since the reqwest-based senders won't even build for that target.

Fixes #281

@ColonelThirtyTwo
In http::sender::params, use http crate instead of reqwest
a962397 
The types in reqwest are re-exports of the types in http, so this should not break
anything, and is needed to make reqwests an optional dependency.
@ColonelThirtyTwo
Refactor into common Pending type
fae77d4 
Almost every module in client::requests defines their own Pending struct,
wrapping a Boxed future, using copy+pasted code. This patch implements a
templated Pending type in the client::requests module, and uses it in submodules.

The patch also sets up a type alias called Pending in each of the submodules,
to retain backwards compatibility.
@ColonelThirtyTwo
Refactor node sniffing
3cc17ad 
Move the implementations of NextParams for SniffedNodes into the sender's
corresponding files, since they fit better there.

Remove (a)sync_next, since it couples the SniffedNodes object too tightly to the
senders. Instead, senders can call `next_or_start_refresh` on the SniffedNodes object
to get the address or a new `Refresher` struct indicating that a refresh is needed.
The sender can then send out the request for the node list, and when the sender gets the
results, it can feed them to the Refresher, consuming it. This makes the SniffedNodes
object flexible with how senders receive their results.
@ColonelThirtyTwo
Add RequestInner trait, and implement it on request types
c2384f7 
The trait specifies the Request and Response types of the request, and specifies
a function to convert the request builder to the Request type.

All of the request builder types already had an `into_request` function, so this
trait just codifies that behavior.

Note that the raw request does not implement this (yet) since it does not do a
standard conversion.

This patch doesn't have any uses of the trait yet, but future patches will make
use of it.
@ColonelThirtyTwo
Generate IntoEndpoint for request types
dc6dcc0 
The request types can only be converted into one concrete type of Endpoint,
but the existing From trait implementation can't convey that, making it impossible
to write a bound for a type that can be converted into an endpoint with any body
type.

The new IntoEndpoint trait fixes this by making the body an associated type.
The bound can be written as, for instance, `Req: for<'a> IntoEndpoint<'a>`
@ColonelThirtyTwo
Generic sending support
a173049 
This adds and changes things to allow external implementations of `Sender`
to exist, and to have a unified send function for results (currently
implemented alongside of the existing send functionality, but will replace it).

This patch adds TypedSender<T>, a companion trait for Sender that also
lets the client handle deserialization, instead of having every request
type implement deserialization for the two available senders.
@ColonelThirtyTwo
Remove individual request's send methods and replace it with generic_…
ec49293 
…send
@ColonelThirtyTwo
Move Pending to http::asynchronous and clean up some of its usage
d94d864
@ColonelThirtyTwo
Features for SyncSender and AsyncSender
c610228 
Client code can now choose whether to include the sync sender or
async sender via Cargo's features system. By default both are included.

Make public some crate-private types, a) to avoid unused warnings, and b)
other crates will likely need them for custom sender implementations.

Had to downgrade the url crate to be compatible with reqwest's version.
@ColonelThirtyTwo
Add Method::to_name for getting a string name from a method
59e5cbc
@ColonelThirtyTwo
Make HttpRequest fields public
063edc0 
Custom backends need access to the fields
@ColonelThirtyTwo
Create a public constructor for Client
68444ed 
Allows custom backends to create a client
@ColonelThirtyTwo
Remove NextParams, moving functionality to Sender::next_params
6250ac2 
The trait requires implementing on types that this crate exports, like SniffedNodes<TSender>
and its parent NodeAddresses<TSender>, but that's not possible to do in external crates, due
to rust's orphan rules.

The return type, once again, is heavily dependent on the sender, since SniffedNodes needs to
do a potentially asynchronous http request. So just cut out the middle man and fold the code
into the Sender trait.
@ColonelThirtyTwo
Make error::request public
f0ddf2e 
Backends in other crates will want to use that function.
@ColonelThirtyTwo
Remove sender from SniffedNodes
354f265 
It's not needed anymore, now that the Sender itself is responsible for querying
for nodes.
@ColonelThirtyTwo
Fix unused macro warning
72aa708
@ColonelThirtyTwo
Copy link
Contributor Author

FYI I haven't tested this too hardly yet, but I'm planning on working on a project that makes use of this crate on WASM, so that should exercise all the functionality.

@mwilliammyers
Copy link
Collaborator

Wow! Thanks for the PR! This is pretty big. So does this allow implementing/creating your own Sender as discussed in #281?

I tried updating to futures 0.3 yesterday and it is a doozy... I wonder if this could help? Would it be crazy if we just remove sync sender now that async/await is stable? I digress and I'll open a different issue to discuss further.

@ColonelThirtyTwo
Copy link
Contributor Author

Yes, you can implement your own sender in your own crate. Example for wasm using stdweb: https://gitlab.com/colonelthirtytwo/elastic-stdweb-rs

IMO I'd rather keep syncsender around; tokio and the async stuff is a bit heavy and I would not be surprised if people would not want to include all that.

@mwilliammyers
Copy link
Collaborator

Any chance you can rebase this onto master so we can run the tests? I assume they might need to be updated too?

@mwilliammyers mwilliammyers mentioned this pull request Nov 13, 2019
Open
@ColonelThirtyTwo
Copy link
Contributor Author

Could do. Just wanted a look-over of the code beforehand in case I had to change anything.

@ColonelThirtyTwo
Copy link
Contributor Author

One issue I've noticed is that since send is now implemented on RequestBuilder, there is no documentation for send on the request type's pages, which isn't very helpful.

@ColonelThirtyTwo
Copy link
Contributor Author

ColonelThirtyTwo commented Nov 15, 2019 •
edited
Loading

...and now with the new version, cargo keeps pulling in reqwests even with default-features: false set... humm Nevermind my fault with a dependency.

@mwilliammyers
Copy link
Collaborator

I am just now starting to look more at this and I have a few initial questions:

  1. Am I to understand that when Generic Associated Types stabilizes we won't need both the Sender & TypedSender<TReqInner> traits? It will just be the Sender trait with an associated type that indicates the type of the sender (WASM vs async etc.)
  2. Is it worth adding your WASM implementation into this crate behind an off by default feature? It doesn't look that big/complicated and might be nice to have...?
  3. Should we make not including sync-sender the default and just make async-sender the default? reqwest is going this route with their next breaking change release (see: seanmonstar/reqwest@7e3c1bc). Our next release v0.21.0 will have a few breaking changes in it already. Granted this would be a much bigger breaking change but might be worth it? I guess it doesn't pull in any extra dependencies so it might not matter?

I'll add more to my review asap...

@ColonelThirtyTwo
Copy link
Contributor Author

  1. Correct. The Sender trait could have a type InProgressResponse<Response>;, which is Result<Response, Error> for SyncSender, Pending<Response> for AsyncSender, and Promise<Response, Error> for StdwebSender. Then typed_send could be moved to the Sender trait and return Self::InProgressResposne<SomeResponseType>.
  2. I'd rather polish the stdweb sender a bit more then include it in a future PR. There are some things I'd like to change with it.
  3. Huh, didn't know that reqwests was turning off the blocking stuff by default now. Doesn't the async stuff require setting up an executor of some sort? Won't that be inconvenient for simple programs? Or did that change with async stabilization?

@mwilliammyers
Copy link
Collaborator

  1. Nice! That seems very clean.
  2. Makes perfect sense. 👌
  3. The alpha crate is setup like that and I would assume the final release will be too; I haven't seen any Indication otherwise. Yeah, it is kinda annoying to setup (although it should be as easy as a #[tokio::main] or #[runtime::main]—assuming the prerequisite dependencies are all downloaded and in order) and then using async/await. I bet their thinking is that while it is a bit more annoying, it kinda forces the ecosystem into using async I/O which is a benefit in performance and utility in the long run...? But I can't speak for the reqwest devs of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

unable to impl Sender due to constraint on elastic::client::private::Sealed
2 participants
@ColonelThirtyTwo @mwilliammyers