Bug Fix: Propagate `managerWrapException` setting in `mkManagerSettingsContext'`
## 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](https://github.com/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`.
合并状态:未合并 6 条评论