Fix issue where readSource may not be resumed

[jdeff]

Issue

When using GCDAsyncSocket, we have noticed that our reads can sometimes
hangs before all of the data is read. The end result is that our read
times out and the connection is closed. This issue seems to be similar
to ones that others have reported (e.g. #89, #185, #225, and possibly #456)

The problem seems to be that doReadData can get into a state where the
method finishes, we have not read all of the bytes for the current read,
but we do not resume the readSource, preventing doReadData from
being triggered again.

Example

Here is a confusing but valiant attempt to explain the issue: When we
enter doReadData we calculate the estimatedBytesAvailable. This
value includes how much data we think is available on the socket plus
how much data is available on some of our internal buffers. If that
number is greater than 0, we will initialize a waiting flag to false.
We then proceed to read from the socket, since there are bytes
available. This is where it gets tricky. We calculate the number of
bytesToRead from the socket by taking the minimum of A.) The
estimatedBytesAvailable plus 16 kb or B.) The remaining bytes of the
current read. The trickiness comes from the fact that when B is larger
than A and the socket has the extra 16kb all ready to be read, then
the call to SSLRead will return with no error. This is problematic
because we rely on a result of errSSLWouldBlock in order to toggle the
waiting flag to true. Finally, we reach the end of the
doReadData method and we have not completed the current read and
waiting is not set to true. Because waiting is false, we do not
attempt to resume the readSource. Since the readSource may be in a
suspended state, there is nothing to trigger another call to
doReadData. This results in what looks like a "hung" or "frozen" state
from the client. Even though data is flowing to our socket, we never
finish reading it out.

This fix in this commit, as simple as it is, just sets waiting to true
in the case of "read to length" and "read to term" where we
are expecting to see more data.

Alternatives

Another possible fix could be to pass the remaining bytes of the current read to
SSLRead. This would then trigger the errSSLWouldBlock result. The issue with this is that I
don't fully understand the logic around
optimalReadLengthWithDefault:shouldPreBuffer:. It may be more
desirable to not trigger that error and just wait until the readSource
says that there is more available from the file descriptor.

Another possible fix would be in how we initially set waiting.
Instead of setting it to !hasAvailableBytes, you could change it to
`(estimatedBytesAvailable + [preBuffer availableBytes]) < (currentRead->readLength -
currentRead->bytesRead). I am not sure if this is desireable and I
would need some input from the maintainers.

[chrisballinger]

Thanks for the investigation! Is it possible to reproduce this via a failing test case?

[jdeff]

@chrisballinger I can give it a shot. We've been able to reproduce this scenario pretty frequently in our iOS app. I'll take a look at the other tests and see if I can come up with something.

[jdeff]

@chrisballinger I put up the test. Let me know what you think. The test is currently only added to the project in Tests/iOS.

If you comment out line 5500 in GCDAsyncSocket.m then you can see it fail. Here is the output of a failed run on my machine:

Test Case '-[CocoaAsyncSocketTestsiOS.GCDAsyncSocketReadTests test_whenBytesAvailableIsLessThanReadLength_readDoesNotTimeout]' started.
/Users/jdeff/Source/Personal/CocoaAsyncSocket/Tests/Shared/GCDAsyncSocketReadTests.swift:20: error: -[CocoaAsyncSocketTestsiOS.GCDAsyncSocketReadTests test_whenBytesAvailableIsLessThanReadLength_readDoesNotTimeout] : failed - Socket was disconnected
<unknown>:0: error: -[CocoaAsyncSocketTestsiOS.GCDAsyncSocketReadTests test_whenBytesAvailableIsLessThanReadLength_readDoesNotTimeout] : Asynchronous wait failed: Exceeded timeout of 0.5 seconds, with unfulfilled expectations: "Read data".
/Users/jdeff/Source/Personal/CocoaAsyncSocket/Tests/Shared/GCDAsyncSocketReadTests.swift:26: error: -[CocoaAsyncSocketTestsiOS.GCDAsyncSocketReadTests test_whenBytesAvailableIsLessThanReadLength_readDoesNotTimeout] : XCTAssertEqual failed: ("0") is not equal to ("102400") - 
Test Case '-[CocoaAsyncSocketTestsiOS.GCDAsyncSocketReadTests test_whenBytesAvailableIsLessThanReadLength_readDoesNotTimeout]' failed (0.603 seconds).

[jdeff]

Looks like the test passed on Travis:

GCDAsyncSocketReadTests
    ✓ test_whenBytesAvailableIsLessThanReadLength_readDoesNotTimeout (0.038 seconds)

[chrisballinger]

Wow awesome work! That test scaffolding will definitely be helpful going forward as well.

[chrisballinger]

@jdeff Please take a look at #600 and see if you can resolve these the test failures. Fortunately the failure isn't related to your waiting = YES patch itself, just the tests you added.

Thank you!