http2-client icon indicating copy to clipboard operation
http2-client copied to clipboard

Dodgy exception handling in `runHttp2Client`

Open parsonsmatt opened this issue 2 years ago • 2 comments

It's possible for the _close to never be called in the event of an exception. These lines of code are especially problematic:

runHttp2Client conn encoderBufSize decoderBufSize initSettings goAwayHandler fallbackHandler mainHandler = do
    (incomingLoop, initClient) <- initHttp2Client conn encoderBufSize decoderBufSize goAwayHandler fallbackHandler
    withAsync incomingLoop $ \aIncoming -> do
        settsIO <- _initSettings initClient initSettings
        withAsync settsIO $ \aSettings -> do
            let client = Http2Client {
              _settings            = _initSettings initClient
            , _ping                = _initPing initClient
            , _goaway              = _initGoaway initClient
            , _close               =
                _initStop initClient >> _initClose initClient
            , _startStream         = _initStartStream initClient
            , _incomingFlowControl = _initIncomingFlowControl initClient
            , _outgoingFlowControl = _initOutgoingFlowControl initClient
            , _payloadSplitter     = _initPaylodSplitter initClient
            , _asyncs              = Http2ClientAsyncs aSettings aIncoming
            }
            linkAsyncs client
            ret <- mainHandler client
            _close client
            return ret

Fortunately the fix should be relatively straightforward - doing mainHandler client `finally` _close client should fix that _close won't be run if mainHandler throws an exception.

parsonsmatt avatar Nov 01 '22 16:11 parsonsmatt

This explains something I've run into I think. Thanks for catching it. If you want to do a PR I can merge it ASAP. Otherwise I'll see if I can get to it tonight. Haven't worked on this for a bit now so need to get it set up otherwise I'd do it now.

ProofOfKeags avatar Nov 03 '22 18:11 ProofOfKeags

oh good catch! your patch proposition should work

I'll try something over a lunch next week.

lucasdicioccio avatar Dec 11 '22 14:12 lucasdicioccio