Reading lines from a non-blocking reader

Question

I want to read \n terminated, UTF-8 encoded lines from a network connection and process each full line. I cannot use fgets, because that function does not support nonblocking IO, which I want to use.

I currently have the following code, but it looks a bit too confusing to me. Some tips on how to simplify the code would be appreciated.

#include <array>
#include <string.h>

// Read lines from a non-blocking reader (e.g. an O_NONBLOCK socket).
// Every complete line is sent to a consumer function, without a newline or null terminator.
// Lines larger than BufSize are silently discarded.
//
// Reader should have approximately this signature:
//   ssize_t reader(void *buffer, size_t maxSize);
//   Should return 0 on EOF, -1 on error, otherwise the number of read bytes.
//   In practice, this should just call read() on a file descriptor.
// Consumer should have approximately this signature:
//   void consumer(const char *start, size_t size);
template<size_t BufSize>
class LineBuffer {
    std::array<char, BufSize> _buffer;
    size_t _bytesBuffered {0};
    size_t _bytesConsumed {0};
    bool _discardLine {false};
public:
    template<typename Reader, typename Consumer>
    size_t readLines(const Reader& reader, const Consumer& consumer)
    {
        char *buf = _buffer.data();
        while (true) {
            auto bytesRead = reader(buf + _bytesBuffered, _buffer.size() - _bytesBuffered);
            if (bytesRead <= 0) {
                return bytesRead;
            }
            _bytesBuffered += bytesRead;

            char* separator = nullptr;
            do {
                char* lineStart = buf + _bytesConsumed;
                separator = static_cast<char*>(memchr(
                    lineStart,
                    '\n',
                    _bytesBuffered - _bytesConsumed));

                if (separator) {
                    size_t lineLength = separator - lineStart;
                    if (!_discardLine) {
                        consumer(lineStart, lineLength);
                    }
                    _bytesConsumed += lineLength + 1;
                    _discardLine = false;
                }
            } while (separator);
            resetBuffer();
        }
    }
private:
    void resetBuffer() {
        size_t bytesRemaining = _bytesBuffered - _bytesConsumed;
        if (bytesRemaining == _buffer.size()) {
            // line too long
            _discardLine = true;
            _bytesBuffered = 0;
            _bytesConsumed = 0;
        } else if (bytesRemaining > 0 && _bytesConsumed > 0) {
            // move the last partial message to the front of the buffer, so a full-sized
            // message will fit
            memmove(_buffer.data(), _buffer.data() + _bytesConsumed, bytesRemaining);
            _bytesBuffered = bytesRemaining;
            _bytesConsumed = 0;
        }
    }
};

// Small test
#include <iostream>
#include <string>

LineBuffer<16> buffer;
void feed(const char *testData) {
    auto end = testData + strlen(testData);
    buffer.readLines(
        [&](void *buf, size_t maxSize) {
            auto size = std::min((ssize_t) maxSize, end - testData);
            memcpy(buf, testData, size);
            testData += size;
            return size;
        },
        [](const char *line, size_t size) {
            std::cout << std::string {line, size} << std::endl;
        });
}

int main() {
    // expected output: one, two, three, four, five, 0123456789abcde.
    feed("one\ntwo\n");
    feed("thr");
    feed("ee\n");
    feed("four\nThis line should be discarded, because it is too long.\n");
    feed("five\n0123456789abcde\n");
}

Answer 1

           if (bytesRead <= 0) {
                return bytesRead;
            }

How can an unsigned type be less than zero?

static_cast<char*>(memchr(

Why aren't you using std algorithms? You want to find the '\n' character in the text, right? That just std::find(begin_iterator, end_iterator, '\n'). No casting.

memmove(_buffer.data(), _buffer.data() + _bytesConsumed, bytesRemaining);

again, use std::copy or copy_n, not memmove.

For whatever functions you do use, include the proper headers for them. I see you have <string.h> but you should not use the C headers in C++. Use <cstring> instead.

⧺SL.io.50 Don't use endl.

If you are expecting this to be launched using async or similar, and it runs until the stream is closed or errored or whatever, then returning a value is not very useful: who are you returning it to? As noted earlier, it will always return 0, so it's useless to have this return value.

---

I don't understand how your loop works.

do {
  // read stuff
  if (separator) { ... }
while (separator);

I thought it would keep reading until it found the end-of-line, but this keeps looping until it doesn't read something with a linebreak.

I agree it's complex, because of your looping and condition structure. You have state variables that need to be understood in order to understand the flow. It could be written in a simpler way as a single loop with clearer conditions.