search

SparkPost / elixir-sparkpost

SparkPost client library for Elixir https://developers.sparkpost.com
Apache License 2.0
44 stars 13 forks source link

Use HTTPoison :recv_timeout option #26

Closed davidefedrigo closed 7 years ago

davidefedrigo commented 7 years ago

See https://github.com/SparkPost/elixir-sparkpost/issues/25.

Test coverage is still 100%.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 989896e536e7860315076e138740ec8bad32df8a on primait:ISSUE-25 into ecbf17a4c2463b41d7b997b7870ff9db0c9bc20b on SparkPost:master.

davidefedrigo commented 7 years ago

Hi there, anyone? Am I the only one with this problem?

ewandennis commented 7 years ago

Hi there @davidefedrigo - deepest thanks for the contribution and sorry I missed this. IMO your PR is useful even if you're the only one with the issue.

It took me a moment to grok the definition of :recv_timeout - it's the timeout waiting for recv() I guess rather than the "timeout used with receiving a connection" as noted here.

Anyway, lets get it merged.

ewandennis commented 7 years ago

@davidefedrigo Looking again at this, you have changed the meaning of :timeout from "connection timeout" to "recv timeout". To avoid the breaking change, I'm think I'm going to make your socket recv() timeout into an :http_recv_timeout and leave the original semantics in place. I hope that small change won't have too large an impact on your own code.

davidefedrigo commented 7 years ago

Yes, I changed back that meaning on purpose because it was the original one, when HTTPotion was in use... if I didn't misunderstand it! :) When I upgraded to HTTPoison I started to get timeout errors, and that was the clue that led me to open the issue.

Anyway, your change it's pretty fine with me. Thanks for notifying me, I just need to update the config of a single project. ;)

Thank you again!