Adding default timeout to Snap REST client

[iwankgb]

Fixes issue of infinitely blocking HTTP connections (mitigates #1482 and similar issues).

Summary of changes:

• added default timeout of 10 seconds to secure and insecure transports

Testing done:

• added a functional test for the timeout feature

It does not seem to be possible to use opts parameter of client.New() as client's http` property is unexported and therefore unavailble from another packages.

@intelsdi-x/snap-maintainers

[marcin-krolik]

LGTM

[IRCody]

Does it make sense to add this as a const somewhere so that it can be edited in one place or potentially modified via config in the future?

[iwankgb]

Constant created.

[IRCody]

What's the advantage of FocusConvey here?

[iwankgb]

None. I have not cleaned the code up after writing a test, apparently. Removed.

[IRCody]

It does not seem to be possible to use opts parameter of client.New() as client'shttp` property is unexported and therefore unavailble from another packages.

Since the metaOpt is defined in the client package it has access to the clients unexported http property, unless I'm misunderstanding something. I also think we can use the http.Client's Timeout property rather than the transports since the transport can be shared among clients and we don't want to enforce identical timeouts (necessarily) across all clients. Something like this inside client.go:

func Timeout (t time.Duration) metaOp {
        return func(c *Client) {
               c.http.Timeout = t
       }
}

Then you would call into the New function with:

c, _ := client.New("address", "", true, client.Timeout(10*time.Second))

Does this seem like it would solve your problem @iwankgb?

[IRCody]

Just leaving this so it's not merged before you have a chance to respond to my comment re: using a metaOpt.

[iwankgb]

I was originally thinking about using metaOp in our application code but adding such a function in client package would solve the issue indeed. Do you want to have such a function implemented and default timeout setting removed?

[IRCody]

@iwankgb: Yes I think that's a little cleaner than the my original thought about just having the const. I also like the idea of using the http.Client timeout instead of the Transport since modifying the Transport would affect all clients.

[iwankgb]

OK :)

[IRCody]

LGTM @iwankgb. Could you squash to a single commit so it can be merged?

[iwankgb]

Thanks a lot, @IRCody :)