Add configurable timeouts

[tolysz]

This commit adds:
waiProxyToSettingsWithTimeouts
Which is a copy of the waiProxyToSettings but it get timeout bounds from the getDest call, and uses timeout from keter.
The idea is that we might need to have different time-outs depending on say destination.

Apply hlint and stylish-haskell suggestions.

[snoyberg]

Instead of adding another function, I'd rather see this become part of the settings. I'd imagine it would be a Maybe (WAI.Request -> IO (Maybe Int, WaiProxyResponse)) field, with the behavior being that if the field is Just it overrides the current timeout setting. Yes, this is ugly, and why in the future we should use setter functions instead of field accessors (which I recently wrote about here: https://github.com/commercialhaskell/haskelldocumentation/blob/master/content/designing-apis-for-extensibility.md).

[tolysz]

I can see that there might be some other proxy settings which we want to override per connection, getDest :: Wai.Request -> IO (LocalProxySettings, WaiProxyResponse) but currently have one setting there:
LocalProxySettings = LocalProxySettings {lpsTimeBound :: Maybe Int}.

As for Maybe (.. getDest..), we can show proxy not setup error if it is still Nothing when we run.

Now if we can break clients, we can have one function doing all that i.e. we drop
simple getDest :: WAI.Request -> IO WaiProxyResponse support from waiProxyToSettings signature.
But I would still add waiProxyFromSettings or some other name which should be stable regardless of which options we might add later.

[snoyberg]

I'm trying to parse the comments and the PR, but my brain's having trouble keeping up (it might be the cold). Firstly, the naming doesn't make sense to me (waiProxyFromSettings). But I'm not longer sure what the motivation is. I thought this was allowing the timeout to be configurable by request, which I thought could be addressed by a single added field. Instead, there's a new function, (somewhat) partial values, and a new datatype. (I think I get your point about the datatype, however.)

I'm also not certain what what you mean by "breaking clients." Do you mean users of the API?

I think the purpose of the new function has something to do with efficiency and not needing to dispatch on the request value twice. But it seems like that could be handled by simply including a Maybe WPRResponse field in LocalWaiProxySettings. But I'm really not certain of the motivations here.

[tolysz]

The idea was to be able to modify proxy settings based on the request.
The original parameter getDest was not allowing to differentiate settings without setting the new whole waiProxyToSettings.
What I wanted was that new getDest will return new proxy settings for a particular request together with WaiProxyResponse allowing per request fine tuned settings.

[snoyberg]

I understood that goal, and I thought my recommendation addressed that:

Instead of adding another function, I'd rather see this become part of the settings. I'd imagine it would be a Maybe (WAI.Request -> IO (Maybe Int, WaiProxyResponse)) field, with the behavior being that if the field is Just it overrides the current timeout setting.

I don't understand the content of the following commit that was added, which seemed to do something very different.

[tolysz]

OK, so let me understand it: the new use should be:

   waiProxyToSettings undefined def
    { wpsGetDest = Just getDest
    } manager

or something safer then undefined?

[snoyberg]

I'd personally use something safer than undefined (or at least more description like error "First argument to waiProxyToSettings forced, even thought wpsGetDest provided").

[tolysz]

Hi, just wonder which tests can I run to confirm that those changes does not break older code, nor lock resources.
The modified keter with those settings run really nice for my setup; the the default timeouts have been killing streaming large files and now there is a possibility to have a stanza for those.

[snoyberg]

Actually, I just wasn't aware that you'd updated the PR. Is your question a polite way of letting me know you think this is ready to be merged?

[tolysz]

Not really, I want to test it better. It improves my use cases.
However It might still need addition of mkLocalWaiProxySettings.

[snoyberg]

For breaking older code, there's nothing automated, just code review. I'm not sure which resources you're worried about locking, can you clarify?

[tolysz]

The idea was to move timeBound from keter down here. Now there is no explicit timeBound on the Right branch of waiProxyToSettings (there is only one buried deep inside HC.responseTimeout).

So I wanted to see if this change is still OK with snoyberg/keter#29

[snoyberg]

I think it's still OK

[tolysz]

I squashed commits, and used Since 0.4.2 as a provisional

[snoyberg]

I merged this to a separate branch and added one more commit on top:

f1584df

Would that work for you?