Rework of timeout errors

[907th]

This PR:

1. Add missing support for read_timeout, open_timeout options to the http adapter.
2. Add support for new write_timeout option to all adapters.
3. Remove duplicate integration tests from spec/httpi/adapter.
4. Add adapater/integration tests for timeout options.
5. Extends all raised timeout exceptions with HTTPI::TimeoutError.

HTTPI::TimeoutError is a module now (it was a class before this PR). I think that this is a only breaking change.

[coveralls]

Coverage increased (+0.1%) to 97.118% when pulling 5dcf101 on 907th:http_timeouts into 878faee on savonrb:master.

[rogerleite]

Hi @907th, thanks for the PR, this is good stuff and my opinion is to approve and merge.

HTTPI::TimeoutError is a module now (it was a class before this PR). I think that this is a only breaking change.

My concerns are:

• If someone extends HTTPI::TimeOut on their project, this change will break the code?
• Just a bump to version 2.5.0 is enough to warn about this?
• @pcai what do you think? Do you see any problem about this breaking change and bumping version?

[pcai]

Thank you for the PR! @rogerleite is correct to point out this breaking change could be a potential concern; we can't anticipate what depends on HTTPI::TimeoutError being a class. Are you able to provide some more background on why the change to a module was made, and how much work it'd be to revert it if needed?

[907th]

@rogerleite

If someone extends HTTPI::TimeOut on their project, this change will break the code?

Yes. And if someone raises HTTPI::TimeoutError their code will broke too. There is one such usage I found with guthub search.

Just a bump to version 2.5.0 is enough to warn about this?

Yes, I think.

@pcai

Are you able to provide some more background on why the change to a module was made, and how much work it'd be to revert it if needed?

The main goal was to introduce a single exception class for all different adapater-specific timeout errors. I found that httpi already employ the following approach: it catches connection errors and extends them with a HTTPI::ConnectionError module (e.g here and in several other places). Pros of this approach: errors retain their original classes, but users can rescue HTTPI::ConnectionError => e because e.kind_of?(HTTPI::ConnectionError) == true. So I decided to use this approach for the TimeoutError too.

To be honest, I've never seen such an approach before. Typically, if I need to wrap several errors with a single error class I just raise ErrorWrapperClass from rescue blocks. If we just reraise all adapter-specific timeout errors using raise TimeoutError that would be breaking change too.

=============

I think I should split this PR into three parts:

1. Fix http adapter support for read/open_timeout + add tests for these options.
2. Add support for write_timeout config option.
3. Wrap adapter-specific timeout errors with a TimeoutError.

so you could merge them separately. Part 1 is just a bug fix, Part 2 - enhancement without breaking changes, and only Part 3 will introduce a breaking change. I'll try to find time next week to do that.

[rogerleite]

@907th the split plan seems very good.

1. Fix http adapter support for read/open_timeout + add tests for these options.
2. Add support for write_timeout config option.

We can merge the 1 and 2 on 2.4.x version.

3. Wrap adapter-specific timeout errors with a TimeoutError.

TimeoutError improvement we merge on 2.5.x and describe it how to deal with TimeoutError module. What do you think @pcai?

Thanks and good job! 👍

[907th]

I have split this PR into:

part 1 - #196
part 2 - #197
part 3 - #198

They are rebased one onto another. Closing this one for now.