http server has no timeouts

[JonasDoe]

If I'm not missing something here, but it seems this in this code there are no timeouts for read, read header and write set ... and I don't see a way to set these values manually. This means the http (server) client is prone to all kind of faulty requests which keep the connection open, and this is outside the scope of what a request handler or message handler could deal with.

[JonasDoe]

I've created an illustration. There's nothing CE-specific about it, but it also uses http.server without timeouts set, despite using a context with timeout in the ServeHTTP function. I've added a timeout middleware (from github.com/go-chi/chi) for the sake of completeness, but it won't help either. So running the test will result in:

=== RUN   TestSlowRequest
--- PASS: TestSlowRequest (5.08s)
=== RUN   TestSlowRequest/with_timeout
2020-01-09 19:54:03.100472 +0100 CET m=+0.005953111: test start:
2020-01-09 19:54:04.105123 +0100 CET m=+1.010591159: context deadline exceeded
2020-01-09 19:54:08.174483 +0100 CET m=+5.079896314: test end
    --- PASS: TestSlowRequest/with_timeout (5.07s)
PASS

Process finished with exit code 0

So it still took the whole 5 seconds, despite ServeHTTP getting canceled after 1 second, because it's hanging at the finishRequest-method in http/server.

[n3wscott]

You can inject the http client and setup timeouts if you need those.

[JonasDoe]

If I understand correctly the client is used to do requests, not to receive them. That's what the server is used for. Looking at this method it seems that the client (or its timeouts) is not regarded at all for server aspects.

So the client timeouts won't do anything if a faulty or malevolent process sends a broken (CE) request to the server (for example the Content-Length could larger than the actual length of the content, or the processing of the server's response could be delayed infinitly). In both cases the server would wait forever.

[n3wscott]

Ah!! yes, you are correct, sorry. Looks like we need to take a look at how to inject a server in there too so you have full control over this.

[JonasDoe]

Yes, that would be great! :) It's not just an issue here by the way, you can find it even in the standard Go library. Just look at methods like http.ListenAndServe(...) with no way set a server (and therefore timeouts) at all ... so you can't use them on a live system.

[JonasDoe]

I see that you're doing more and more stuff internally with the server (like enforcing an ochttp.Handler, so it does not really seem that you want the server to be injected and be in conflict with your internal setup. Maybe a http.Transport/http.Protocol option dedicated for timeouts, setting dedicated config fields (similar to the addr field) would be the nicer way to go now? That said, I don't feel confident enough to contribute something myself atm since it looks like everything is just getting reworked for v2.0.0 and I don't know about your master plan.

[slinkydeveloper]

Hi @JonasDoe , Now things are settled down a bit and we're preparing for the final 2.0 release, Are you willing to provide a PR for the timeouts on server?

[JonasDoe]

Sorry for the delay, I've resorted to a workaround via reflections and forgot about this issue. I haven't checked the new code yet, but I think I could provide a PR. Any opinions on how granular the protocol options should be? Like one timeout to bind them all, or should we respect all the different http.Server timout settings? I'ld prefer the "single timout" approach since nobody except me seems to miss that feature at all, so there's no reason to make it overly sophisticated. It's not like all other http.Server settings are available via the protocol options anyway. And: What about a default timeout?

[JonasDoe]

Seems to be fixed with an DefaultTimeout by now.

[embano1]

@JonasDoe can you share ref/link to this?

[JonasDoe]

Sure! Here you go https://github.com/cloudevents/sdk-go/blob/main/v2/protocol/http/protocol_lifecycle.go#L43C17-L43C17

[embano1]

Ah, sorry I thought you were referring to this being fixed in Go stdlib 😀

All good then!

[JonasDoe]

Ah, sorry I thought you were referring to this being fixed in Go stdlib 😀

All good then!

Oh, that would be great but will probably never happen b/c it would break broken designs! :D

[nkreiger]

@JonasDoe QQ, how do you override that default to extend it? I see its hardcoded as a const, trying with difficulty on the best approach to override that, thanks

[JonasDoe]

@JonasDoe QQ, how do you override that default to extend it? I see its hardcoded as a const, trying with difficulty on the best approach to override that, thanks

In the past, I set the timeout values via reflection. After the hardcoded "fix", I did not change anything and just relied on that constant value.