Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Support for binary requests. #38

Closed
wants to merge 15 commits into
from

Conversation

Projects
None yet
5 participants

benbro commented Jan 25, 2011

Hi

I've added support for binary requests.

Changes:

  1. Added support for {binary_body, true} option in mochiweb_http:start/1.
    If set, mochiweb_http:loop/3 sets the socket to {packet, http_bin} instead of {packet, http}.
  2. Added support for retrieving the path, qs params and post parameters
    when the socket is set to {packet, http_bind}.

I've tested it in my app and it is working good.
Could you please add this functionality?
Am I missing something else that is needed to support binary requests?

Thanks

Owner

etrepum commented Jan 25, 2011

This code has zero tests, so I can't accept it as-is.

Changing the behavior of parse_qs/1 is unacceptable, unfortunately. I'm sure there is code out there that is passing binaries to parse_qs and expecting strings as a return. We should have a separate module or different names for functions that are expected to work with binaries. Overall I like the idea of having a full binary path, but I can't accept this implementation.

benbro commented Jan 25, 2011

I'll create get(binary_path), parse_binary_qs and parse_binary_post to mochiweb_request that will use a new module mochiweb_binary_util.
That way we can add this functionality as "experimental" without affecting anything else.
Is this what you mean?

I'll add some tests.

Owner

etrepum commented Jan 25, 2011

Yeah, you can do that, although if the input is in binary to begin with maybe there should also be a different module for mochiweb_request too... the client code won't have to change really, because the module name is wrapped up in the "instance".

benbro commented Jan 25, 2011

If we want a separate mochiweb_binary_request module we need to pass from loop, Binary=true/false to mochiweb_http:request and than pass it to mochiweb_http:header.
header will create new_request or new_binary_request
Is this ok?

I think that HEADERS are always atoms and strings but I'm not sure.

Owner

etrepum commented Jan 25, 2011

There's a packet mode that will return headers as binary, but there is still a fixed list of commonly used headers that are encoded as atoms.

benbro commented Jan 25, 2011

Atoms are better than binaries.
The packet mode is probably httph_bin.
Do we need a binary module for cookies as well or is it ok as iolist?

Owner

etrepum commented Jan 25, 2011

The most important parts are the headers, URL, and forms. Cookies are a bit less important

Atoms are better, but mochiweb_headers doesn't currently have any optimizations to use them effectively

benbro commented Jan 25, 2011

I've implemented a binary version for request, headers, cookies and util as you suggested.
{binary_body, true} sets the request to binary. Missing option defaults to normal request.

Headers and cookies are handled as binaries.

I've updated the tests and they all pass.

Do you want me to duplicate all the do_GET and do_POST tests for binaries? It will duplicate the time requires to run the test.

Thanks

Owner

etrepum commented Jan 26, 2011

It's better to have more tests... if they get too slow we can always look at ways of fixing that, but it's more important for the code to be correct right now :) Sounds like you've been doing some great work so far, I look forward to looking more closely (but am very busy today)

Member

lemenkov commented Mar 17, 2011

The sad truth is that we can't merge this pull request in its current state. For example, it seems that changes introduces in several commits (2496be8, 1fd9b55, 1227cb8, b359081) have been discarded by the next one. Adding them into this pull request only makes things more complicated for the reviewer.

Could you, please, rebase these patches on top of current master, drop useless ones, review them again and merge together ones which are tightly connected to each other (representing the same logical change) if any, and I'll review them asap.

I do believe that mochiweb will benefit from this change, but I did several attempts to review it and failed miserably so far.

benbro commented Mar 21, 2011

Basically, I added new option binary_body to mochiweb_http. It also applies to the header so it might use binary instead of binary_body. If it's a binary request we are using {packet, http_bin} for the body and headers and calling mochiweb:new_binary_request instead of new_request. I added mochiweb_binary_request, mochiweb_binary_headers, mochiweb_binary_util and mochiweb_binary_cookies to support binary requests. I also added binary tests to all the modules and to mochiweb.erl. I'm not sure I can separate it to several stand alone commits.

I am guessing these never made it into in? It would definitely be a nice addition.

Owner

etrepum commented Dec 11, 2011

It's something we're still considering but you'd probably be better off switching to a different web server that already natively uses binary throughout since this is an API breaking change anyway.

Contributor

mworrell commented Sep 19, 2012

Are there any performance differences with/without the binary patches?

@etrepum etrepum closed this Jul 10, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment