search

kazu-yamamoto / http2

HTTP/2.0 library including HPACK
BSD 3-Clause "New" or "Revised" License
86 stars 23 forks source link

`allocSimpleConfig` should take a timeout parameter, and timeouts should be optional #112

Open edsko opened 7 months ago

edsko commented 7 months ago

At the moment, allocSimpleConfig hardcodes the TimeManager timeout to 30 seconds:

https://github.com/kazu-yamamoto/http2/blob/b8f2268f5079667c1d6f47dfb254056b5b277823/Network/HTTP2/H2/Config.hs#L20

This should instead be a parameter, and that parameter should not be of type Int, but rather of type Maybe Int: not applications want to impose timeouts between messages sent/received. For example, when running gRPC over HTTP2, it's entirely conceivable that there might be very long time intervals between messages on an open connection. Indeed, these intervals could be an hour or longer, which means that on 32-bit systems at least there is no appropriate value that can be passed to System.TimeManager.initialize that would work.

I will open a corresponding ticket also in http2-tls.

edsko commented 7 months ago

@kazu-yamamoto I'm not entirely sure yet if I am blocked on this; on 64-bit machines at least I can workaround the problem of not having optional timeouts by initializing my own TimeoutManager with a really high timeout. I will get back to you.

If it turns out I am blocked on this, I am happy submit a PR with a fix, but in the case I guess we need to think about what a suitable design would be.

kazu-yamamoto commented 1 week ago

eda08bb should fix this.

edsko commented 1 day ago

eda08bb0f92522c7f227cdd750c6c704e4aef203 only partially addresses this: although it makes the timeout configurable, it does not make it optional. Timeouts can be disabled on 64-bit machine by setting the timeout really high, but on 32-bit no value is high enough (max timeout would be roughly 30 mins).

I am not sure what the rationale is for the timeouts, but perhaps one way forward is to implement keep-alive pings (#149), and just like the timeout is "reset" in the current http2 whenever any activity takes place, also reset it whenever a keep-alive ping gets a response? In that case we could set the keep-alive pings to say 10 minutes, the timeout to 15 mins, the workaround with really large timeouts would not be required anymore.

That said, making the WAI timeout manager optional would be at least one way to make the race condition in https://github.com/kazu-yamamoto/http2/issues/153#issuecomment-2495369997 go away, at least in deployments that don't need the WAI manager (like grapesy).

kazu-yamamoto commented 23 hours ago

eda08bb0f92522c7f227cdd750c6c704e4aef203 only partially addresses this: although it makes the timeout configurable, it does not make it optional. Timeouts can be disabled on 64-bit machine by setting the timeout really high, but on 32-bit no value is high enough (max timeout would be roughly 30 mins).

Sorry but I did not understand your request correctly. OK, you want to disable the timeout action. So, what about having a special value, 0, to disable the timeout?

I am not sure what the rationale is for the timeouts, but perhaps one way forward is to implement keep-alive pings (#149), and just like the timeout is "reset" in the current http2 whenever any activity takes place, also reset it whenever a keep-alive ping gets a response? In that case we could set the keep-alive pings to say 10 minutes, the timeout to 15 mins, the workaround with really large timeouts would not be required anymore.

Ping is good. Please go ahead.

That said, making the WAI timeout manager optional would be at least one way to make the race condition in https://github.com/kazu-yamamoto/http2/issues/153#issuecomment-2495369997 go away, at least in deployments that don't need the WAI manager (like grapesy).

I believe the race is already fixed in forkManagedTimeout with the lock of IORef Bool.