-
Notifications
You must be signed in to change notification settings - Fork 16
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
edge-http: make fields in {Req,Resp}Headers non-optional #37
Conversation
4cfcb88
to
5e71bae
Compare
Ensures that the {Req,Resp}Headers is always complete and adheres to the HTTP spec. Saves a lot code from dealing with `Option`s. Also, `send_status_line` is removed, and inlined into `send_request` and `send_status`.
5e71bae
to
0e4687d
Compare
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.
My main concern is as expressed below - we are deepening an already existing memory consumption problem. Headers
, with N = 60 is 60*8 = 480 bytes, which when created in a future often double its size due to Rust not being very efficient wr.t. futures' size.
We must have a way to pre-create RequestHeaders
and ResponseHeaders
, or else we won't be able - in future - to pre-create them outside the async code and then possibly pool them. We are also removing this existing option for users who would like to do it, and who are not using the Connection
machinery altogether.
Thanks for the review! I understand your point. I would like to know, in the first place, do you think that making the fields non- |
It certainly makes the code simpler and easier to reason about. (My one little concern is the Otherwise - between introducing something like Open to any other suggestions though. |
This might indeed be unsolvable, other than using How about this:
|
I don't see any other way either.
|
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.
Please restore all const constructors and the calls to those. :-)
The rest looks fine.
Did not know this about |
Ensures that the {Req,Resp}Headers is always complete and adheres to the HTTP spec. Saves a lot code from dealing with
Option
s.Also,
send_status_line
is removed, and inlined intosend_request
andsend_status
. This cuts ~80 lines from the code, which is nice.