Bug Fix: Propagate managerWrapException setting in mkManagerSettingsContext'

[adithyaov]

Summary

mkManagerSettingsContext' replaces a managerWrapException in the ManagerSettings, which prevents the managerWrapException from the input settings from being propagated, resulting in the loss of input behaviour.

This PR is a bug fix.

Problem description

Functions of interest:

• managerWrapException in ManagerSettings.
• mkManagerSettingsContext'.
• newTlsManagerWith.

managerWrapException

Consider the setting managerWrapException in the ManagerSettings. It's a modifier of type:

forall a. Request -> IO a -> IO a

The assumption is that the action configured here is triggered at a certain point in the execution flow. (Where it is triggered is irrelevant to the issue).

mkManagerSettingsContext'

mkManagerSettingsContext' does a lot of things, but the relevant part is the following.

mkManagerSettingsContext' set mcontext tls sockHTTP sockHTTPS = set
    ...
    , managerWrapException = \req ->
        let wrapper se
              | Just (_ :: IOException)          <- fromException se = se'
              | Just (_ :: TLS.TLSException)     <- fromException se = se'
#if !MIN_VERSION_tls(1,8,0)
              | Just (_ :: TLS.TLSError)         <- fromException se = se'
#endif
              | Just (_ :: NC.LineTooLong)       <- fromException se = se'
              | Just (_ :: NC.HostNotResolved)   <- fromException se = se'
              | Just (_ :: NC.HostCannotConnect) <- fromException se = se'
              | otherwise = se
              where
                se' = toException $ HttpExceptionRequest req $ InternalException se
         in handle $ throwIO . wrapper
    }

There is an input argument set :: ManagerSettings. This function returns the modified ManagerSettings using the input ManagerSettings.

The important thing to note here is that the managerWrapException from the input ManagerSettings is ignored and overwritten.

This means, if we had previously configured managerWrapException and created ManagerSettings. Using mkManagerSettingsContext' on the ManagerSettings we created will overwrite the configured functionality.

newTlsManagerWith

newTlsManagerWith takes an input ManagerSettings and returns a Manager. newTlsManagerWith uses mkManagerSettingsContext' on the input ManagerSettings before creating a Manager. So the configured managerWrapException is lost when the Manager is created.

Changes made

The change in this PR modifies mkManagerSettingsContext' so that it uses the managerWrapException from the input ManagerSettings.

Side effects of this change

mkManagerSettingsContext' was originally idempotent; this change breaks that idempotency.

The only issue I see is that multiple applications of mkManagerSettingsContext' will nest the handler in managerWrapException. This occurs when we do something like newTlsManagerWith tlsManagerSettings.

Verification

The test case added will fail without the change but will succeed with the change.

More context on why this is required

We are using cachix/hs-opentelemetry-instrumentation-http-client for integrating hs-opentelemetry.

This package hooks telemetry (hs-opentelemetry) into the HTTP manager. The pre-hook is inserted into managerWrapException, and the post-hook into managerModifyResponse.

[sol]

@adithyaov sorry, I'm still a little bit lost here. Am I right to assume that you consider this a bug fix (as opposed to a new feature)?

As for cachix/hs-opentelemetry-instrumentation-http-client, is it currently completely broken, or does it work in certain situations, but not in others? Can you explain in which situations it works, and what you are doing differently so that it does not work for you?

I capture that mkManagerSettingsContext' ignores managerWrapException and that this is something that we might want to address. But every bug report and every feature request starts from the user's perspective, and I'm missing the user's perspective here.

Also, do you currently have any workarounds? (e.g. can you achieve what are you're trying to do by setting mWrapException on the returned Manager?)

[adithyaov]

@sol Yes, sorry, this is a bug fix. This isn't a new feature.

cachix/hs-opentelemetry-instrumentation-http-client works as expected when http-client is used and breaks when http-client-tls is used.
The hooks inserted into managerWrapException and managerModifyResponse should be executed for things to work properly. mkManagerSettingsContext' ignores managerWrapException, thereby breaking the behaviour.

But every bug report and every feature request starts from the user's perspective, and I'm missing the user's perspective here.

I don't understand what you mean by the user's perspective. Could you please elaborate?

Also, do you currently have any workarounds? (e.g. can you achieve what are you're trying to do by setting mWrapException on the returned Manager?)

A workaround is possible, but this PR is primarily a bug fix. The only reason I've added more context on our usage is to motivate this fix. I should've worded it better.

[sol]

But every bug report and every feature request starts from the user's perspective, and I'm missing the user's perspective here.

I don't understand what you mean by the user's perspective. Could you please elaborate?

Something like

To use cachix/hs-opentelemetry-instrumentation-http-client I need to create a Manager with a modified mWrapException. I tried to use newTlsManagerWith, which didn't seem to work... I think the underlying reason is in mkManagerSettingsContext'... as a workaround I am setting mWrapException directly. ... this change is not critical to me as I have a workaround, but I am still proposing it so that other users can benefit from it.

I think I have all the information I need now. But for the future, something like this would tell me:

• what exactly are you trying to achieve
• how exactly did you try to achieve it
• how critical is this for you, is there a workaround (that's important to determine priority)

[sol]

Side effects of this change

mkManagerSettingsContext' was originally idempotent; this change breaks that idempotency.

The only issue I see is that multiple applications of mkManagerSettingsContext' will nest the handler in managerWrapException. This occurs when we do something like newTlsManagerWith tlsManagerSettings.

@adithyaov do you think this is observable?

[Yuras]

I think we have the same issue with other settings as well, e.g. managerRetryableException, see #496

[odr]

I think that the best way here is

• making new defaultManagerSettingsTLS from defaultManagerSettings with only managerWrapException rewritten
• use defaultManagerSettingsTLS everywhere in HTTP.Client.TLS instead of defaultManagerSettings
• drop rewriting of managerWrapException from mkManagerSettingsContext'

In this case we will be consistent with non-TLS client:

• we have default behavior as expected
• we can set custom managerWrapException and use default version there

This bug is really annoying...

UPD: But in this case the old code with newTlsManagerWith defaultManagerSettings{..} will be broken silently. It is bad...