allocSimpleConfig should take a timeout parameter, and timeouts should be optional

[edsko]

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

http2/Network/HTTP2/H2/Config.hs

Line 20 in b8f2268

timmgr <- T.initialize $ 30 * 1000000

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]

@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]

eda08bb should fix this.

[edsko]

eda08bb 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 #153 (comment) go away, at least in deployments that don't need the WAI manager (like grapesy).

[kazu-yamamoto]

eda08bb 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 #153 (comment) 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.

[edsko]

So, what about having a special value, 0, to disable the timeout?

If you prefer that over Maybe Int, that would be fine with me also.

Note that we are not blocked on this, we can workaround the problem on 64-bit machines, and the client does not require support for 32-bit machines, so it's not urgent. However, the ping support may be required; I might have some time to implement that, maybe early next year.

[edsko]

I think yesodweb/wai#1017 will close this.

[kazu-yamamoto]

Let's close.

[edsko]

Thanks @kazu-yamamoto :)