closeConnection can swallow IO exceptions and block / hang indefinitely

[liminalisht]

amqp/Network/AMQP/Internal.hs

Lines 418 to 431 in 3ddd972

	closeConnection :: Connection -> IO ()
	closeConnection c = do
	CE.catch (
	withMVar (connWriteLock c) $ \_ -> writeFrame (connHandle c) $ Frame 0 $ MethodPayload $ Connection_close
	--TODO: set these values
	0 -- reply_code
	(ShortString "") -- reply_text
	0 -- class_id
	0 -- method_id
	)
	(\ (_ :: CE.IOException) ->
	--do nothing if connection is already closed
	return ()
	)

closeConnection swallows any IO exceptions it might encounter upon sending the close connection message to the server, and then waits on the server indefinitely, expecting the server to send back a connection_closed_ok message.

One problem I see is that if an IO exception is encountered when sending the server the close connection message, there is no reason to expect that the server has properly received the message, and therefore no reason to expect that the server will respond with a connection_closed_ok message.

In such a case, the MVar contents will never become full, and the call to readMVar will block forever. From the readMVar docs:

Atomically read the contents of an MVar. If the MVar is currently empty, readMVar will wait until it is full.

https://www.stackage.org/haddock/lts-18.22/base-4.14.3.0/Control-Concurrent-MVar.html#v:readMVar

I would propose that closeConnection at a minimum be altered such that readMVar is not called if an IO exception is encountered.

[hreinhardt]

Is this a theoretical concern or can you actually trigger this as a bug?

I'm asking because I believe the code is correct, based on these two assumptions:

• The only way that writeFrame could fail is if the connection is closed.
• When the connection is closed (explicitly or by some connection error), we always fill connClosedLock:

  amqp/Network/AMQP/Internal.hs

  Line 304 in 3ddd972

  void $ tryPutMVar ccl ()

In addition, I think most users will have heartbeats enabled, so that would be another safeguard to prevent infinite blocking.

[liminalisht]

I have in fact experienced closeConnection hanging (and have temporarily gotten around it by invoking it using timeout.)

However, to be clear, I do not know why closeConnection hangs. The explanation I proposed above is indeed (to your point) theoretical. I proposed it because I could not think of another reason for the function hanging.

Given the code you pointed me to, I am now quite perplexed. It looks like the lock should definitely be filled whenever the connection is closed.

That leaves me wondering whether there's really no other way for writeFrame to fail (other than the connection being closed).

[hreinhardt]

After thinking about it some more, you might be right. If writeFrame fails without closing the connection, the code might deadlock. I have no idea if that could ever happen, but it's probably better to be resistant to that.

I've pushed a fix: ff169bb

Does the change look good to you?

[liminalisht]

Yeah, looks good to me. Thanks for the quick response / work!

[hreinhardt]

Ok, I've pushed a new version to Hackage. Curious to see if the blocking issue persists for you.