movio / bramble

A federated GraphQL API gateway
https://movio.github.io/bramble/
MIT License
497 stars 55 forks source link

Lack of server timeouts #161

Closed aerfio closed 2 years ago

aerfio commented 2 years ago

Http servers created in https://github.com/movio/bramble/blob/24b50dca339947b8e1424d9f70af15d3299a7284/main.go#L66-L69 don't have anything set in their "timeout" field which may lead to various problems. Zero value means no timeout

Links:

If you want I can prepare a PR which sets those defaults to

    // DefaultReadTimeout sets the maximum time a client has to fully stream a request (5s)
    DefaultReadTimeout = 5 * time.Second
    // DefaultWriteTimeout sets the maximum amount of time a handler has to fully process a request (10s)
    DefaultWriteTimeout = 10 * time.Second
    // DefaultIdleTimeout sets the maximum amount of time a Keep-Alive connection can remain idle before
    // being recycled (120s)
    DefaultIdleTimeout = 120 * time.Second

but I'd like to first hear what do you think about it. Maybe some different values, maybe somehow expose it via configuration json?

pkqk commented 2 years ago

@aerfio good catch, we haven't hit any issues with the timeouts not being specified yet but that's probably luck more than anything.

An initial PR setting to recommended defaults would be appreciated, and if we do find a need to tweak them we can add the ability to configure them.