Test cleanups for timeout and async usage

[nh2]

Minor test cleanups for #413 and #286.

CC @ocheron

[kazu-yamamoto]

Excellent!

[ocheron]

This PR modified how the test suite reports failures.

Example before:

client exception: HandshakeFailed (Error_Protocol ("FFDHE group is not secure enough",True,InsufficientSecurity)), supported: Supported {supportedVersions = [TLS12], supportedCiphers = [DHE-RSA-AES128-SHA1], supportedCompressions = [0], supportedHashSignatures = [(HashIntrinsic,SignatureRSApssRSAeSHA256),(HashSHA384,SignatureRSA),(HashIntrinsic,SignatureEd25519),(HashSHA512,SignatureRSA),(HashSHA1,SignatureRSA),(HashSHA256,SignatureRSA),(HashIntrinsic,SignatureRSApssRSAeSHA384),(HashIntrinsic,SignatureEd448),(HashIntrinsic,SignatureRSApssRSAeSHA512),(HashSHA1,SignatureDSS)], supportedSecureRenegotiation = False, supportedClientInitiatedRenegotiation = False, supportedExtendedMasterSec = AllowEMS, supportedSession = True, supportedFallbackScsv = True, supportedEmptyPacket = True, supportedGroups = []}
server exception: HandshakeFailed (Error_Packet_unexpected "Alert [(AlertLevel_Fatal,InsufficientSecurity)]" " expected: handshake"), supported: Supported {supportedVersions = [TLS12], supportedCiphers = [DHE-RSA-AES128-SHA1], supportedCompressions = [0], supportedHashSignatures = [(HashSHA1,SignatureRSA),(HashIntrinsic,SignatureRSApssRSAeSHA256),(HashSHA512,SignatureRSA),(HashIntrinsic,SignatureEd25519),(HashSHA256,SignatureRSA),(HashIntrinsic,SignatureEd448),(HashIntrinsic,SignatureRSApssRSAeSHA384),(HashSHA1,SignatureDSS),(HashSHA384,SignatureRSA),(HashIntrinsic,SignatureRSApssRSAeSHA512)], supportedSecureRenegotiation = False, supportedClientInitiatedRenegotiation = False, supportedExtendedMasterSec = AllowEMS, supportedSession = True, supportedFallbackScsv = True, supportedEmptyPacket = True, supportedGroups = [FFDHE3072,FFDHE2048,X25519,FFDHE3072]}

Now becomes:

client exception: HandshakeFailed (Error_Protocol ("FFDHE group is not secure enough",True,InsufficientSecurity)), supported: Supported {supportedVersions = [TLS12], supportedCiphers = [DHE-RSA-AES128-SHA1], supportedCompressions = [0], supportedHashSignatures = [(HashSHA1,SignatureDSS),(HashIntrinsic,SignatureEd25519),(HashIntrinsic,SignatureRSApssRSAeSHA256),(HashSHA384,SignatureRSA),(HashIntrinsic,SignatureEd448),(HashSHA512,SignatureRSA),(HashIntrinsic,SignatureRSApssRSAeSHA512),(HashSHA1,SignatureRSA),(HashSHA256,SignatureRSA),(HashIntrinsic,SignatureRSApssRSAeSHA384)], supportedSecureRenegotiation = False, supportedClientInitiatedRenegotiation = False, supportedExtendedMasterSec = AllowEMS, supportedSession = True, supportedFallbackScsv = True, supportedEmptyPacket = True, supportedGroups = []}
server exception: AsyncCancelled, supported: Supported {supportedVersions = [TLS12], supportedCiphers = [DHE-RSA-AES128-SHA1], supportedCompressions = [0], supportedHashSignatures = [(HashSHA1,SignatureDSS),(HashIntrinsic,SignatureEd448),(HashIntrinsic,SignatureRSApssRSAeSHA256),(HashSHA256,SignatureRSA),(HashSHA512,SignatureRSA),(HashIntrinsic,SignatureRSApssRSAeSHA384),(HashSHA1,SignatureRSA),(HashIntrinsic,SignatureRSApssRSAeSHA512),(HashIntrinsic,SignatureEd25519),(HashSHA384,SignatureRSA)], supportedSecureRenegotiation = False, supportedClientInitiatedRenegotiation = False, supportedExtendedMasterSec = AllowEMS, supportedSession = True, supportedFallbackScsv = True, supportedEmptyPacket = True, supportedGroups = [P256,P384,FFDHE4096]}

[ocheron]

If no better solution, could those changes be reverted please?
Obfuscation of test diagnostics impacts a normal development workflow.
Fixing an issue like #438 need precisely to check if alert messages are accepted and decrypted by the peer or not.
I don't care about the relative merits of function X or Y, but breaking silently the code I submitted removes desire to contribute again.

[nh2]

If no better solution

@ocheron Sorry, I didn't understant from your previous comment in #416 (comment) that there was a problem; it looked like just a remark, not like an issue, so I didn't reply at that time.

breaking silently the code I submitted removes desire to contribute again

I still don't quite understand what the problem is though / which code was broken. Do you mean that after this change, the AlertLevel_Fatal,InsufficientSecurity is no longer raised from the server, but instead AsyncCancelled is shown, and that you desire to see both the client-side and the server-side error messages?

What this PR did is to make it such that only the first error of client or server is raised, and the respective other thread is killed.

I thought that this was the inteded behaviour, so that you get onl the first error message, and not other errors that may be caused by the first one but are not the root causes.

If you would like to get both the client and the server errors, the correct solution is most likely to change

      let readResult = waitBoth cAsync sAsync >> readChan resultQueue
      cont (writeChan startQueue, readResult) 

into something like

      let readResult = do
            eClientRes <- try $ wait cAsync
            eServerRes <- try $ wait sAsync
            case (eClientRes, eServerRes) of
              (Right _clientRes, Right _serverRes) -> return () -- no error
              (Right _clientRes, Left serverErr) -> throwIO serverErr -- only the server errored
              (Left clientErr, Right _serverRes) -> throwIO clientErr -- only the client errored
              (Left clientErr, Left serverErr) -> throwIO serverErr $ ... -- make an exception here that contains both the client and server errors
            readChan resultQueue
      cont (writeChan startQueue, readResult) 

Is this what you are after?

[ocheron]

Clearly that would be an improvement w.r.t. the level of details reported after a failure.

But the intent is also to handle a scenario where one side fails and the other side hangs because it never receives the message it expects.
i.e. let the test fail as soon as one side of the connection gets an error in order to run as many tests as possible before the 10-min timeout on Travis CI.

[nh2]

let the test fail as soon as one side of the connection gets an error

@ocheron That's what should be happening with this PR merged though -- did you observe that to not work?

[ocheron]

If I summarize my requirements:

• (a) test failure reported promptly after client or server error (any one)
• (b) ability to see if the other end of the connection terminated after 1st failure and how (not an AsyncCancelled status)
• (c) a global test timeout on top of that, in case of no failure but client+server deadlock, or bug in the test infrastructure itself
• (d) a single version of the test suite, no manual editing to get property (a) or (b), but both always

Then:

• async + wait client + wait server: will not do property (a)
• withAsync + waitBoth: does not do property (b)
• withAsync + waitBoth + anything else: will not do property (b)
• async + waitBoth: did both (a) and (b)

[nh2]

Sorry, I had forgotten to delete the line waitBoth cAsync sAsync from my snippet above (done now); would the withAsync + wait approach I suggested above work?

A global test timeout should be easily addable with the timeout function (if the test driver doesn't already do it).

[ocheron]

That will do property (b) but not (a):
If one of client or server fails while sending its message, the other keeps waiting.
And the test case hangs in one wait call or the other.

Package async was introduced to get property (a) in test cases already doing (b) and (c).

Property (a) + (b) together is all about keeping one child thread running, so not withAsync.

[nh2]

• (a) test failure reported promptly after client or server error (any one)

• (b) ability to see if the other end of the connection terminated after 1st failure and how (not an AsyncCancelled status)

If promptly means "immediately", you can really only get 1 of (a) and (b):

Either you wait until one side fails and cancel the other one, or you wait until both sides fail.

If promptly means after some timeout, then I think you could just do

let readResult = timeout yourTimeout $ do

[ocheron]

So it's a negative answer about the revert.
Thank you, I'll port my tiny codebase to a different language, hopefully there will be no need to learn cryptography and TLS internals to do simple HTTPS requests :-)