Skip to content
This repository has been archived by the owner on Nov 9, 2017. It is now read-only.

Implement send method to send content along with the request #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

flaper87
Copy link

@flaper87 flaper87 commented Oct 5, 2013

No description provided.

let mut request = ~RequestWriter::new(Post, FromStr::from_str("http://httpbin.org/post")
.expect("Uh oh, that's *really* badly broken!"));

request.send("Post It!".as_bytes());
Copy link
Owner

Choose a reason for hiding this comment

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

"Post It!".as_bytes() is more efficiently written bytes!("Post It!").

Copy link
Author

Choose a reason for hiding this comment

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

+1 Thanks for the advice! Changed it in the new commit

@emberian
Copy link
Contributor

@flaper87 tests failed

@emberian
Copy link
Contributor

(Due to the ToPrimitive/FromPrimitive conversion)

@flaper87
Copy link
Author

@chris-morgan ping?

@chris-morgan
Copy link
Owner

@flaper87 Sorry I haven't followed this and #9 up; I got quite busy for a while, finishing up Uni. I need to figure out the direction I want rust-http to go in to best meet the requirements of the various classes of users, including (but certainly not limited to!) Servo; thus far the work that has been done has been very largely experimental; I think it's time for a bit of deliberate (rather than accidental) design before progressing much further; I think rust-http will change a fair bit in the process. If these two will be of immediate value to people, I'm open to merging them as they are, but don't expect things to stay the same; quite a lot of rust-http is likely to be rewritten.

(I've looked in some detail at Spray's design; it's the closest thing extant to what I wish to achieve, though there are still very distinct differences in what we need. As with everything other such project, it's inspiration only, not a copying exercise. Rust needs to end up with the best HTTP libraries for just about every type of task, which is a rather large undertaking!)

@flaper87
Copy link
Author

@chris-morgan Hey, no worries! Thanks for reaching back.

I share your thoughts w.r.t making rust-http the best http library for every task. In my opinion, the API has to expose different layers of abstraction:

  • Lower level
  • Higher level (Basically the one I worked on)
  • Higher level and concurrent

It is important to keep them separate and not hide any of those layers from users. So, since this is already a start and the work has been done, I'd say we should merge it and perhaps use it as a reference for the future refactor process.

That being said, I'm willing to help with the new design so, if you agree, we could work on this together.

@marcbowes
Copy link

Something that bothers me at the moment is that RequestWriter is trying to be too many things. It does DNS lookups, socket management, will implement some of the HTTP spec and manage headers (like ContentLength). The fact that some things which are notoriously brittle (like DNS resolution) happen in a constructor is very concerning.

I think a good first step would be to separate out Request from RequestWriter. This may help answer some questions such as what to do if the user wrote the headers out without (or with a bad) ContentLength (the answer being: "request writer is a low level API and if you call it directly we provide no guarantees you're following the protocol").

@flaper87
Copy link
Author

@marcbowes I had the same thought when I started using rust-http. I think the final API, as mentioned in my previous comment, should be split in 3 different levels. The first allows the user to do anything but if something goes wrong it the user's fault. The second one, gives a bit less control but it gives more guarantees and reliability. Finally, there could also be a concurrent API on top of the previous 2.

The Reques trait you mention, in my proposal, would be part of the second level.

@marcbowes
Copy link

It's also worth considering that some users are going to want to get hooks into the client. For example, you may want do install a hook that injects an HTTP header. We should dog-food on this extensibility early on in the design process. Concrete examples would be ContentLength and UserAgent: both should be installed as part of the request pipeline, rather than some hard-coded entity. Users should be able to remove the default handlers and replace them with their own.

A concrete use-case I have in mind is talking to AWS services (S3 and so forth). To implement this, we'd need to do operations on the request body and headers "just before" sending the request.

@flaper87
Copy link
Author

Agreed. This patch flaper87@dadcca0 allows users to hack the request header and still not having to mess with the RequestWriter directly.

@marcbowes
Copy link

Nice 👍

@chris-morgan: I think it's time for a bit of deliberate [..] design before progressing much further

Do you think it's worthwhile putting together some sort of proposal/comparison?

One other consideration is testing. It would be nice to be able to the client to write to any stream (possibly many at once). This could also be useful in production.

@flaper87
Copy link
Author

@marcbowes it'd be cool to have some sort of proposal. IIRC @chris-morgan was working on one.

I kinda think we should let this 2 patches land and re-factor on top of them.

@marcbowes
Copy link

Yup. The code-base is still small enough for this not to matter much (not saying your code is bad; just emphasizing that the risk is low :-)).

@jdm
Copy link

jdm commented Mar 13, 2014

@chris-morgan It would be nice if we could move forward with this in some way - either close this PR with some kind of clear details on what a preferred API would be, or merge and iterate.

metajack pushed a commit to metajack/rust-http that referenced this pull request Aug 9, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants