Shouldn't body be closed by decoder ?

[zig]

I want to implement a cancellable post method. However for context.Done() to be notified, the body has to be previously closed, at least in some cases.

When an endpoint method is called, just after the decoder was called, unless SkipRequestBodyEncodeDecode is used, the body can't be accessed anymore, it would thus make sense to call Close() on it before calling the endpoint method.

Currently, to implement a cancellable method, I need to use SkipRequestBodyEncodeDecode and explicitly close the body (which I can now access, even though I didn't need it). But this is incompatible with using authentification (I opened another issue regarding this), and seems like a workaround rather than a clean solution, since I didn't need the body. Beside, not closing the body seems to lead to some resource leaks in some cases (might be related to golang/go#43966), so closing it in all cases seems to be safer.

EDIT: To clarify, this issue happens when the body of the request is not read (no parameters were declared there so goa doesn't bother decoding the body). If the body is read, everything works normally without needing to close the body.

[raphael]

Interesting question :) The Go http server should take care of closing the body but that will happen after the handler returns. Can you point me to where the requirement for the body to be closed before the context can be cancelled comes from?

[zig]

I am not sure why it is like that, but if we don't close the body, then we don't always get notified of the client cancelling on his side. It is not always the case, if just merely testing by directly connecting on the service with curl, it does work as expected (no need to close the body), but when we are in actual deployment behind a routing proxy (traefik in our case), not only we don't get notified, but even worst there seem to be leak of connections.
It's easy to miss the issue, because for it to happen, the client need to close his side before the method returned, so if all methods respond quickly, it won't happen very often. In our case though we have a long running POST method, and it happens all the time.

[raphael]

Thanks for the explanation, one more question: does implementing a custom decoder that closes the body help? This would be a good test to validate the approach (might also be a better workaround than using SkipRequestBodyEncodeDecode).

[zig]

Thanks for the suggestion, bizarrely I tried and didn't manage to fix the issue with a custom decoder. Maybe there's something I didn't completely understand, the custom decoder is actually a custom decoder factory, it returns a decoder for a given request, I don't fully understand why it is done like that, does it mean that the Decode method could be called more than once for a given decoder ? If so I should not close the body at the end of Decode (in general), because this could break subsequent calls.

Here is what I tried :

type BodyCloser struct {
	request *http.Request
	lower   goahttp.Decoder
}

func (d *BodyCloser) Decode(v any) error {
	err := d.lower.Decode(v)
	d.request.Body.Close()
	return err
}

func NewBodyCloserRequestDecoder(r *http.Request) goahttp.Decoder {
	return &BodyCloser{r, goahttp.RequestDecoder(r)}
}

To complete the explanation, this is actually this issue we hit : golang/go#23262

It is a pretty old issue, while there's a working solution in closing the body, the issue is still open, so I'm not sure what it means, maybe the golang team consider this is not sufficiently documented ?

Anyway, what they are saying in that issue is that if you don't read the body, then you should close it.

[raphael]

Hmm the above code looks good to me. The reason the decoder is a factory and not a specific decoder is so that the code can inspect the request Content-Type header and use the proper decoding strategy (e.g. if an API was to support both JSON and XML). The factory is called on each request so it shouldn't be an issue to call Close on the body. AFAIK the above implements the behavior described in the original message (i.e. closes the body before the handler is called).

[zig]

I see that makes sense.

Regarding fixing by using a custom decoder, it does not work, because there's nothing to decode in the body. That's what triggers the bug in the first place : if the body is not read, you need to close it (or you don't receive cancel), that's what is described in golang/go#23262. But the code generated by goa does not call the decoder in this case, so I can't implement the fix there.

[zig]

I verified, if I add an optional request parameter in the body in my design, then generated code use the decoder, which will read the body, but in this case the bug is not triggered, even without the custom decoder, because the body was read.

So maybe what goa should do (the generated code I guess) is when it does not use the decoder, close the body instead.

[raphael]

That makes sense, would you care putting together a PR? Adding a else to https://github.com/goadesign/goa/blob/v3/http/codegen/server.go#L588 might be a good starting point but there may need to be more changes.

[zig]

I would be happy to propose something. But I don't know what ...

It turns out that when using nginx as proxy, closing the body does not work. I need to make sure the body is read. For example by adding some dummy parameter, so generated code will call the decoder, or even again using SkipRequestBodyEncodeDecode and calling Read on the body instead of Close.

So, I guess for now I will stick to this workaround, and since I can also workaround #3328 , I am not blocked.

(note : reading just partially is not enough, you need to read up to EOF, in my case there was two bytes in the body, reading just one byte did not fix, but reading 100 (which returned only 2 + EOF) did. The two bytes were '{' and '}' )

[raphael]

OK I thought the problem was that Goa wasn't closing the request Body when it is empty. The code I linked above is where the code checks whether the decoder should be called and I was suggesting adding a else clause that would close the body if that check failed (and the decoder wasn't called).

[zig]

Well, I also thought that, but after digging deeper, it is not clear, since it does not resolve the issue when nginx is used as reverse proxy.

What seems to fix the issue is to make sure you consume the body fully. Normally the body should be almost empty (for example in our case it contains an empty json dict {} ). I'm a bit resilient to propose this workaround generically because it's not as straightforward as just closing the body.

Basically, this issue on golang side golang/go#23262 is still open. I am not sure what it means, do they plan to change the behaviour one day ? Or improve the doc ?

[raphael]

Thank you, that makes sense. It does look like the need for reading the request body before a request can be closed is a hard limitation - good to know! Closing this issue since there isn't anything Goa can do to change this behavior.