Skip to content

Add fix for upcoming OTP release, and some other small things #3

Closed
wants to merge 19 commits into from

3 participants

@oscarh
oscarh commented Sep 13, 2011

There is now a dialyzer warning, which I'd like to get rid of, but I also don't like to add code branches which canot be reached... Any suggestions on how to deal with this:
lhttpc_client.erl:577: The pattern {'http_error', Data} can never match the type {'error',_} | {'ok',binary() | maybe_improper_list(binary() | maybe_improper_list(any(),binary() | []) | byte(),binary() | []) | {'http_error',string()} | {'http_request','DELETE' | 'GET' | 'HEAD' | 'OPTIONS' | 'POST' | 'PUT' | 'TRACE' | string(),'*' | string() | {'abs_path',string()} | {'scheme',string(),string()} | {'absoluteURI','http' | 'https',string(),'undefined' | non_neg_integer(),string()},{non_neg_integer(),non_neg_integer()}} | {'http_response',{non_neg_integer(),non_neg_integer()},integer(),string()},binary()}

Magnus Froberg and others added some commits Sep 14, 2010
@uwiger
@oscarh
oscarh commented Sep 13, 2011

Oh, I misinterpreted the dialyzer warning... I thought it was complaining about a clause I didn't match...

The http_error comes from the possible return value {ok, {http_error, Reason}, Tail}. Will push another commit.

@oscarh
oscarh commented Sep 13, 2011

A fix for this was pushed, should really have added a test for it though...

@eproxus

Is this intentional? Have you checked with the OTP team?

I implemented this since no one replied for a day or so...

Now the story might have changed:
http://erlang.org/pipermail/erlang-questions/2011-September/061145.html

@eproxus
eproxus commented Sep 15, 2011

Looks good to me. If the ssl socket options patch is "okay", then we should merge.

@oscarh
oscarh commented Sep 15, 2011

It seems that OTP will actually add support for {packet, httph} at some point, so this workaround might be useless.

http://erlang.org/pipermail/erlang-questions/2011-September/061145.html

@oscarh
oscarh commented Sep 15, 2011

I guess we can chose to remove the ssl:setopts fix (if OTP will include the fix in the next release), but I think there is some value in the rest of the commits. Well, mainly the test with broken trailers.

@eproxus
eproxus commented Sep 16, 2011

In which version does it not exist? Or did it never exist?

@oscarh
oscarh commented Sep 16, 2011

It's missing in "new ssl". This means it doesn't work per default with OTP HEAD from here.

@oscarh
oscarh commented Oct 13, 2011

I think the fix was included in the latest OTP, I'll try to create a new pull request without the workaround this weekend.

@oscarh oscarh closed this Oct 13, 2011
@eproxus
eproxus commented Oct 13, 2011

Ok, good stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.