search

ferd / dlhttpc

dispcount-based lhttpc fork for massive amounts of requests to limited endpoints
Other
36 stars 17 forks source link

Fix connection_timeout kinda-typo #6

Closed loucash closed 9 years ago

loucash commented 9 years ago

@ferd Hey, yet another fix. In Options there is no such thing as connect_timeout.

It is being extracted earlier: ConnectionTimeout = proplists:get_value(connection_timeout, Options, infinity)

Without that we have connection_timeout always being set to infinity.

loucash commented 9 years ago

Heh, I guess this one might not be that easy to merge. If you want to keep backwards api compatibility for those using "bypass", then this fix is invalid. But, if you want to make it prettier and cleaner...

Or maybe it is made on purpose to be compatible with some other API? I don't know, you decide.

ferd commented 9 years ago

Oh, interesting typo. That might have been API preservation there, but the main users of that code I know of have forked it since then I think.

It's mostly used for naming only: https://github.com/ferd/dlhttpc/blob/master/src/dlhttpc_disp.erl#L44 We could probably replace that usage with the connect_timeout one taking precedence, if I read it right.

loucash commented 9 years ago

Yeah. dlhttpc_disp:checkout is using connection_timeout option.

In State connect_timeout option is kept.

dlhttpc:verify_options/2 is prepared for both options: https://github.com/ferd/dlhttpc/blob/master/src/dlhttpc.erl#L584 and https://github.com/ferd/dlhttpc/blob/master/src/dlhttpc.erl#L589.

Grep shows that connect_timeout is used more often, as an option and as an error reason. I would suggest to keep connect_timeout option then. What do you think @ferd ?

ferd commented 9 years ago

Yeah I'd agree. If it's used nowhere but to set up the initial state of the worker when checking out and then for the bypass mode, we should merge things back to use 'connect_timeout' everywhere. When that's out I'll probably have to make this a bump to 0.3 anyway.

Good eye. I think I had done this to respect some old API, but if there's a problem on a new version (I'll probably write a changelog), we can wait for reports and see. The biggest user of that lib I know of otherwise have their own fork.

loucash commented 9 years ago

@ferd sounds good. Can you take a look at ee603b921697205c62a838fb82d10ef097994b1e ?

ferd commented 9 years ago

Cool, thanks!