ITADN

Bug Fix: Propagate `managerWrapException` setting in `mkManagerSettingsContext'`

#566Pull Requestadithyaov 创建于 2025-04-18
A
adithyaovcommented
## 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 条评论