Add an overall request timeout #47
Conversation
…d, the request will be terminated as well, since there is no value in continuing if the writer has been closed.
ghost
dismissed
http/server_test.go
Outdated
| So(resp.StatusCode, ShouldEqual, 200) | ||
|
|
||
| b, err := io.ReadAll(resp.Body) | ||
| So(err, ShouldEqual, nil) |
There was a problem hiding this comment.
I would suggest using ShouldBeNil instead
http/server_test.go
Outdated
| resp, err := http.Get("http://" + a) | ||
|
|
||
| Convey("the request is terminated with a 'connection timeout' response", func() { | ||
| So(err, ShouldEqual, nil) |
There was a problem hiding this comment.
same as above ShouldBeNil
http/server_test.go
Outdated
| go func() { | ||
| if err := s.Server.ListenAndServe(); err != nil { | ||
| if !errors.Is(err, http.ErrServerClosed) { | ||
| os.Exit(1) |
There was a problem hiding this comment.
just a suggestion, please feel free to disagree.
I would create an error channel and pass it as a paramter to startServer (or create it in startServer and return it as a second value, afte rthe func), so that tests can validate what error is returned by ListenAndServe if required. Then instead of exiting on unexpected errors we can just fail the test.
Also, this channel could be closed when the inner go func finishes its execution
If this would make things too complex, feel free to ignore comment.
There was a problem hiding this comment.
No it's a good suggestion - see if the new version conforms
http/server_test.go
Outdated
|
|
||
| return func() { | ||
| if err := s.Server.Shutdown(context.Background()); err != nil { | ||
| os.Exit(1) |
There was a problem hiding this comment.
maybe we can also provide a shutdownErr channel, similarly to the description of above.
|
|
||
| // GetFreePort is simple utility to find a free port on the "localhost" interface of the host machine | ||
| // for a local server to use. This is especially useful for testing purposes | ||
| func GetFreePort() (port int, err error) { |
http/server_test.go
Outdated
|
|
||
| Convey("with a server whose handler runs for less than the server's WriteTimout", func() { | ||
| h := http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { | ||
| time.Sleep(DefaultWriteTimeout / 100) |
There was a problem hiding this comment.
maybe this sleep is not needed? (we are testing the case where the handle finishes fast anyway)
What
Add an overall request timeout so that if a write timeout will be triggered, the request will be terminated as well, and a response written, before the writer is closed, since there is no value in continuing if the writer has been closed.
How to review
Review code
Who can review
Anyone