Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audit http2 for uses of catch from unliftio #193

Closed
edsko opened this issue Jul 17, 2024 · 4 comments
Closed

Audit http2 for uses of catch from unliftio #193

edsko opened this issue Jul 17, 2024 · 4 comments
Labels
bug Something isn't working http2 Problems in `http2` and related packages priority: high Usability of the library severely hindered

Comments

@edsko
Copy link
Collaborator

edsko commented Jul 17, 2024

In kazu-yamamoto/http2#92 we changed things so that the KilledByHttp2ThreadManager exception would not be remain uncaught, but a later change to unliftio meant that the behaviour of catch changed: only synchronous exceptions are caught! We should check all uses of catch and friends in http2.

@edsko edsko added bug Something isn't working priority: high Usability of the library severely hindered http2 Problems in `http2` and related packages labels Jul 17, 2024
@edsko
Copy link
Collaborator Author

edsko commented Jul 17, 2024

Chances are good that this will fix #156.

@FinleyMcIlwaine
Copy link
Collaborator

Searching for SyncOrAsync in unliftio, only try, catch, and handle have this weird behavior, so they might be the only functions we need to audit usages of.
Screenshot 2024-07-17 at 8 37 10 AM

@edsko
Copy link
Collaborator Author

edsko commented Jul 19, 2024

Below is an exhaustive (I think) list of all functions in unliftio that require scrutiny; all of these functions intentionally do not deal with asynchronous exceptions. Auditing would take one of two forms:

  1. Functions that are polymorphic in the exception type, such as

    catch         :: (..) => m a -> (e -> m a) -> m a
    catchJust     :: (..) => (e -> Maybe b) -> m a -> (b -> m a) -> m a

    would depend on how e is instantiated; if instantiated to SomeException, and the intention is to catch also asynchronous exception, this function cannot be used (unliftio offers catchSyncOrAsync).

  2. Functions using SomeException, such as

    catchAny      :: (..) => m a -> (SomeException -> m a) -> m a

    need to be audited to see if the intention really is to catch only synchronous exceptions.

The full list:

catch         :: (..) => m a -> (e -> m a) -> m a
catchAny      :: (..) => m a -> (SomeException -> m a) -> m a
catchDeep     :: (..) => m a -> (e -> m a) -> m a
catchAnyDeep  :: (..) => m a -> (SomeException -> m a) -> m a
catchJust     :: (..) => (e -> Maybe b) -> m a -> (b -> m a) -> m a

handle        :: (..) => (e -> m a) -> m a -> m a
handleAny     :: (..) => (SomeException -> m a) -> m a -> m a
handleDeep    :: (..) => (e -> m a) -> m a -> m a
handleAnyDeep :: (..) => (SomeException -> m a) -> m a -> m a
handleJust    :: (..) => (e -> Maybe b) -> (b -> m a) -> m a -> m a

try           :: (..) => m a -> m (Either e a)
tryAny        :: (..) => m a -> m (Either SomeException a)
tryDeep       :: (..) => m a -> m (Either e a)
tryAnyDeep    :: (..) => m a -> m (Either SomeException a)
tryJust       :: (..) => (e -> Maybe b) -> m a -> m (Either b a)

pureTry       ::         a -> Either SomeException a
pureTryDeep   :: (..) => a -> Either SomeException a

catches       :: (..) => m a -> [Handler m a] -> m a
catchesDeep   :: (..) => m a -> [Handler m a] -> m a

mapExceptionM :: (..) => (e1 -> e2) -> m a -> m a

catching      :: (..) => Getting (First a) SomeException a -> m r -> (a -> m r) -> m r
catching_     :: (..) => Getting (First a) SomeException a -> m r -> m r -> m r
handling      :: (..) => Getting (First a) SomeException a -> (a -> m r) -> m r -> m r
handling_     :: (..) => Getting (First a) SomeException a -> m r -> m r -> m r
trying        :: (..) => Getting (First a) SomeException a -> m r -> m (Either a r)
trying_       :: (..) => Getting (First a) SomeException a -> m r -> m (Maybe r)

edsko added a commit to edsko/http2 that referenced this issue Jul 19, 2024
In kazu-yamamoto#92 we added an exception handler
that was meant to catch _all_ exceptions (sync and async). This got changed in
kazu-yamamoto#114 (specifically,
kazu-yamamoto@52a9619):
when we moved from `Control.Exception` to `UnliftIO.Exception`, we got a
different behaviour for `catch` and friends (see
well-typed/grapesy#193 (comment)) for a
full list. This commit fixes some unintended consequences of this change.
@edsko
Copy link
Collaborator Author

edsko commented Jul 19, 2024

Report and PR at kazu-yamamoto/http2#138 .

edsko added a commit to edsko/http2 that referenced this issue Jul 23, 2024
In kazu-yamamoto#92 we added an exception handler
that was meant to catch _all_ exceptions (sync and async). This got changed in
kazu-yamamoto#114 (specifically,
kazu-yamamoto@52a9619):
when we moved from `Control.Exception` to `UnliftIO.Exception`, we got a
different behaviour for `catch` and friends (see
well-typed/grapesy#193 (comment)) for a
full list. This commit fixes some unintended consequences of this change.
edsko added a commit to edsko/http2 that referenced this issue Jul 24, 2024
In kazu-yamamoto#92 we added an exception handler
that was meant to catch _all_ exceptions (sync and async). This got changed in
kazu-yamamoto#114 (specifically,
kazu-yamamoto@52a9619):
when we moved from `Control.Exception` to `UnliftIO.Exception`, we got a
different behaviour for `catch` and friends (see
well-typed/grapesy#193 (comment)) for a
full list. This commit fixes some unintended consequences of this change.
edsko added a commit to edsko/http2 that referenced this issue Jul 24, 2024
In kazu-yamamoto#92 we added an exception handler
that was meant to catch _all_ exceptions (sync and async). This got changed in
kazu-yamamoto#114 (specifically,
kazu-yamamoto@52a9619):
when we moved from `Control.Exception` to `UnliftIO.Exception`, we got a
different behaviour for `catch` and friends (see
well-typed/grapesy#193 (comment)) for a
full list. This commit fixes some unintended consequences of this change.
@edsko edsko closed this as completed Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working http2 Problems in `http2` and related packages priority: high Usability of the library severely hindered
Projects
None yet
Development

No branches or pull requests

2 participants