vapor-community / sockets

🔌 Non-blocking TCP socket layer, with event-driven server and client.
MIT License
575 stars 54 forks source link

Wrong readAll() implementation? #142

Closed koraykoska closed 6 years ago

koraykoska commented 6 years ago

I stumbled over the readAll() implementation of ReadableStream while reading through the code and for me it looks like the implementation is wrong as the documentation says:

Reads all bytes from the stream using a chunk size.

The current implementation reads bytes until one read call returns chunkSize or more bytes as lastSize will always be set to the byte count of the current read call.

So currently the following happens:

Example 1:

Socket state: --- 1024 Bytes --- -> readAll() -> first read(max: chunkSize) call -> lastSize == 512 -> return 512 read bytes

512 bytes will be returned and not all

Example 2:

Socket state: --- 2 Bytes --- -> readAll() -> first read(max: chunkSize) call -> lastSize == 2 -> try to read more, no more bytes left, lastSize == 0 -> lastSize stays 0 until there are more bytes to read. Will continue to read until there is one block (one read call) which returns 512 bytes.

Conclusion:

The current readAll() doesn't seem right to me. It looks like in many situations readAll() will never return (as there, for example, the socket won't ever have chunkSize bytes but far less).

If this behaviour is intended, sorry for the inconvenience, but if not, the following proposed solution should work.

Proposed solution:

public func readAll(chunkSize: Int = 512) throws -> Bytes {
    var lastSize = 1
    var bytes: Bytes = []

    while lastSize > 0 {
        let chunk = try read(max: chunkSize)
        bytes += chunk
        lastSize = chunk.count
    }

    return bytes
}
koraykoska commented 6 years ago

If the intention is to minimize the read() calls, the following should work:

public func readAll(chunkSize: Int = 512) throws -> Bytes {
    var lastSize = chunkSize
    var bytes: Bytes = []

    while lastSize >= chunkSize {
        let chunk = try read(max: chunkSize)
        bytes += chunk
        lastSize = chunk.count
    }

    return bytes
}

Once read() returns less than chunkSize, we don't need to read again as there are no more bytes.

tanner0101 commented 6 years ago

@Ybrin that implementation looks better. can you create a PR?

koraykoska commented 6 years ago

See #143