Request should not fail on small ReadBufferSize

[hbhoyar-uber]

Can we not fail request if the req/resp header size is greater than the read buffer size.

I can see a bufio bug was mentioned for tryRead function but looks like it's resolved now. Can we have the above mentioned behaviour?

Also, some callback could also be configured to inform that buffer size was low. Library users could emit metrics and increase the buffer size appropriately to have best balance between memory usage and CPU usage.

@valyala Could you review the request?

[erikdubbelboer]

I'm afraid that is not possible with how fasthttp is build. It always parses the whole headers on each try and therefor needs the whole headers in the read buffer. Changing this would require quite a big refactor of the header parsing code.

[hbhoyar-uber]

Thanks @erikdubbelboer for reviewing. I tried to refactor the code with minimal changes to support this and looks like it's working. The code might look ugly but can be refactored better. Can you review it once and if it looks okay, I can raise a PR.

This feature could be enabled by a optional flag and callback could be provided for observability if this case occurs so that the user can tune the buffer size.

        var buf []byte
    for {
        var fullBytes []byte
        h.resetSkipNormalize()
        b, err := r.Peek(n)
        if len(b) == 0 {
            return handlePeekError(r, n, err)
        }

        b = mustPeekBuffered(r)
        if len(buf) != 0 {
                         // create full buffer with existing bytes
            fullBytes = append(buf, b...)
        } else {
                         // first iteration, use only read bytes
            fullBytes = b
        }

        headersLen, errParse := h.parse(fullBytes)
        if errParse == nil {
                         // discard only header bytes read in current iteration
            mustDiscard(r, headersLen-len(buf))
            return nil
        }

        headerErr := headerError("request", err, errParse, fullBytes, h.secureErrorLogMessage)
        smallBufErr := &ErrSmallBuffer{}
        if !errors.As(headerErr, &smallBufErr) {
            return headerErr
        }

        if len(buf) == 0 {
                         // append to existing buffer to release reference of b from bufio.Reader
            buf = append(buf, b...)
        } else {
            buf = fullBytes
        }

                 // discard all currently read bytes
        mustDiscard(r, len(b))
    }

[erikdubbelboer]

I'm pretty sure this is not going to work. You're storing the return value of bufio.Reader.Peek() in buf. But the documentation for bufio.Reader.Peek() says: The bytes stop being valid at the next read call.

[hbhoyar-uber]

I tested this locally and it was working properly.

The return value of Peek() is not directly stored. It is being copied in buf in each iteration. First iteration has special handling to defer copying later if header parsing fails. All other iterations, copy at the start.