Network.TLS.IO: Return errors as Left

[erikd]

Previously these were thrown as exceptions even though the error
types were correct to return then as a Left.

Closes: #220

[erikd]

There is no API change here, but two things that were thown as exceptions from

recvPacket :: MonadIO m => Context -> m (Either TLSError Packet)

namely Error_EOF and Error_Packet are now returned as Left, because both of those constructors were already part of the TLSError type

[ocheron]

Have you checked if the exceptions are rethrown downstream?
Because we may have code reacting on them.

[erikd]

@ocheron "downstream"? You mean in hs-tls or on packages that depend on it?

[ocheron]

Could be anything actually, but first hs-tls yes.
There's code detecting Error_EOF in TLS.Network.Core, so we'll need to test connection termination carefully.

[kazu-yamamoto]

Does this mean that contextRecv in readExact should be protected with catch or something?

[erikd]

I need to look at this more carefully. Will do so some time this week.

[ocheron]

Here's my full review after checking the code path in more details.

What happens in your scenario at https://github.com/erikd-ambiata/test-https-client-resource-vanished/ is that when you kill one end of the connection, at the other end readExact generates Error_EOF or Error_Packet.

setEOF is called from readExact because we can distinguish there an end-of-stream Error_Packet from other Error_Packet exceptions raised by disengageRecord. I don't think it's safe to remove the setEOF call.

Also exception Error_EOF is caught with safeHandleError_EOF, so that recvData returns an empty bytestring. Moving away from exceptions is good, but the behaviour should not change.

I'm not against wrapping an end-of-stream Error_Packet inside a Terminated exception. I have no idea if it's possible to recover from Error_Packet exceptions coming from disengageRecord, so I would prefer not to change that part.

[erikd]

I've updated the PR so that the behavior at the API level of Error_EOF is the same as before. With this patch, Error_EOF is no longer returned as an exception, but as the Left of an either. Now in recvData we check the TLSError and if it is Error_EOF we return ByteString.empty.

[kazu-yamamoto]

This is just my opinion but I don't like IO (Either a).
IO itself means that this computation may fail.
I seems to me that IO (Either a) is redundant.
So, I would like to keep:

readExact :: Context -> Int -> IO Bytes

And I would like to have:

recvRecord :: Bool -> Context -> IO (Record Plaintext)

If advantages of IO (Either a) in this case were explained, I would change my mind.

[erikd]

IO itself means that this computation may fail.

I agree. The problem is that the existing code uses exceptions for flow control. In most languages this is considered a code smell. Exceptions are usually reserved for exceptional circumstances. One of the things that the commit is "fixing" is throwCore Error_EOF. I don't think that anyone could possibly argue that Error_EOF is an "exceptional" circumstance for a network connection.

I consider this the first patch in a series of patches that would remove all throwing of exceptions within TLS, catch all exceptions at the interface between TLS and lower levels and convert them into errors that a represented in the types. TLS would then provide at API of total functions to clients of TLS.

I know Haskell is not a total programming language, but it is possible to program in it as if it is.

The function recvRecord is currently:

recvRecord :: Bool    -- ^ flag to enable SSLv2 compat ClientHello reception
           -> Context -- ^ TLS context
           -> IO (Either TLSError (Record Plaintext))

I agree, the function signature does not express the partiality that result from exceptions, but removing the Either only makes it more partial. I think this function signature should remain as it is, but should add a contract in the comments:

-- This function catches all exceptions and converts them to TLSError. If exceptions escape
-- this function it should be considered a bug that needs to be fixed.
recvRecord :: Bool -> Context -> IO (Either TLSError (Record Plaintext))

[ocheron]

I didn't realize disengageRecord uses throwError and not throwCore.
So going through terminate for all Error_Packet should be OK.

I still think the setEOF call in readExact should be kept there.

[erikd]

I still think the setEOF call in readExact should be kept there.

@ocheron Sorry, I don't understand that comment. I don't see how this PR changes the circumstances in which setEOF is called. In the old code setEOF is not called when Error_EOF is caught. That will still be true after this patch is applied. Do you meant that setEOF should be called on Error_EOF?

[kazu-yamamoto]

I agree with IO Either.

[kazu-yamamoto]

@erikd The current readExact does not call setEOF at all!

[erikd]

@erikd The current readExact does not call setEOF at all!

Yes, I am somewhat confused by @ocheron' s comment which is why I was asking for clarification.

[kazu-yamamoto]

One more item to discuss. Can Record typ ver (Fragment "") express EOF? If so, we can implement:

recvRecord :: Bool -> Context -> IO (Record Plaintext)

[erikd]

@kazu-yamamoto EOF is not the only failure state that is be communicated via the Either (eg there is also Error_Protocol) so its not possible to remove the Either.

[kazu-yamamoto]

@erikd Yes. But other true errors can be thrown.
I quickly hacked to remove Either in my local branch. I needed to modify many code but they got much simpler.

[ocheron]

I see setEOF here:
https://github.com/vincenthz/hs-tls/blob/fe1dbb4129d3f817847b5e461fc43bea174b613a/core/Network/TLS/IO.hs#L41

[kazu-yamamoto]

I finally recognized that Record typ ver (Fragment "") means a record with a header and an empty body, not EOF. So, this approach is not acceptable.

Now I fully support @erikd's approach.

[erikd]

@ocheron You are correct. I will update the PR.

[kazu-yamamoto]

LGTM.

[kazu-yamamoto]

LGTM.

[kazu-yamamoto]

Merged. Thank you for your contribution!

[erikd]

This has been merged. Closing.