openfaas / of-watchdog

Reverse proxy for STDIO and HTTP microservices
MIT License
259 stars 115 forks source link

Wrong implementation of mode serializing #123

Closed mslaga closed 1 year ago

mslaga commented 2 years ago

According to: https://golang.org/src/net/http/request.go?s=12527:12599 ContentLength cannot specify how much data will be copied to process because -1 is the correct value.

Expected Behaviour

Function works when ContentLength is -1

Current Behaviour

In my case, on raspberry pi, the function always fails because of that

Possible Solution

Copy the req.InputReader stream to stdin in loop until EOF is reached.

Steps to Reproduce (for bugs)

Wrong be design https://golang.org/src/net/http/request.go?s=12527:12599

Context

According to the documentation of Http Request it is wrong implementation

Your Environment

Not necessary in this case

alexellis commented 2 years ago

Hi thanks for your interest

Under "Steps to Reproduce (for bugs)" you must provide a full set of instructions on how to reproduce the issue with this specific project and a sample function / workload.

Alex

mslaga commented 2 years ago

Hi, I am sorry, but I do not see a point in a full set of instructions on how to reproduce. It could work perfectly in your case. I do not know when -1 can occur in ContentLength, but it happens in my case.(Maybe when not all body data are available in stream yet?). Sample function will not help you. In the function, stdin is just empty because of-watchdog did not read any content from the request.

I think the code analysis is sufficient in this case. _executor/serializing_forkrunner.go: line 74

if req.ContentLength != nil {
        defer req.InputReader.Close()

https://golang.org/src/net/http/request.go?s=12527:12599

ContentLength records the length of the associated content.
The value -1 indicates that the length is unknown.

Assume that req.ContentLength is equal to -1 as an unknown length, but real data in the body is 20byes.

        limitReader := io.LimitReader(req.InputReader, *req.ContentLength)

Here limitReader will be set to:

type LimitedReader struct { R Reader = req.InputReader N int64 = -1 }

https://pkg.go.dev/io#LimitedReader

A LimitedReader reads from R but limits the amount of data returned to just N bytes. Each call to Read updates N to reflect the new amount remaining. Read returns EOF when N <= 0 or when the underlying R returns EOF.

        var err error
        data, err = ioutil.ReadAll(limitReader) // N in limitReader is -1, so it will return EOF without any data.

data - is empty

alexellis commented 1 year ago

ContentLength cannot specify how much data will be copied to process because -1 is the correct value.

I don't think that's quite right:

    // ContentLength records the length of the associated content.
    // The value -1 indicates that the length is unknown.
    // Values >= 0 indicate that the given number of bytes may
    // be read from Body.

We need full instructions to reproduce any issues that you're facing.

Alex

alexellis commented 1 year ago

@mslaga why are you using the of-watchdog in serializing mode? Who taught you or instructed you to do that?

The classic watchdog should satisfy all your needs, if you just want to buffer all the request and response into memory?

alexellis commented 1 year ago

We will probably make the limit reader only come into action if there is a Content-Length header set, and later consider a hard limit for a maximum, configurable via environment variable.