clientDoDeadline maybe timeout before deadline

[chhquan]

client.go clientDoDeadline routinue (G0) raise a routinue (G1) to recive response, and start a new timer according to deadline。G0 will wait for response and timer。But the connection for reading response will timeout before the deadline, because the connection will be set deadline when client's ReadTimeout/WriteTimeout is set.

//func (c *HostClient) doNonNilReqResp(req *Request, resp *Response) (bool, error) {
	if c.WriteTimeout > 0 {
		// Set Deadline every time, since golang has fixed the performance issue
		// See https://github.com/golang/go/issues/15133#issuecomment-271571395 for details
		currentTime := time.Now()
		if err = conn.SetWriteDeadline(currentTime.Add(c.WriteTimeout)); err != nil {
			c.closeConn(cc)
			return true, err
		}
	}

	if c.ReadTimeout > 0 {
		// Set Deadline every time, since golang has fixed the performance issue
		// See https://github.com/golang/go/issues/15133#issuecomment-271571395 for details
		currentTime := time.Now()
		if err = conn.SetReadDeadline(currentTime.Add(c.ReadTimeout)); err != nil {
			c.closeConn(cc)
			return true, err
		}
	}

[erikdubbelboer]

That's not how SetWriteDeadline and SetReadDeadline work. They don't wait for a full request/response. Instead they are about single packets.
If you SetReadDeadline of 10 seconds. But the server responds with a single byte every second the timeout will never be hit. But it will take you more than 10 seconds to receive the full response.

The time you pass to DoDeadline can also be different from your ReadTimeout or WriteTimeout.

[chhquan]

That's not how SetWriteDeadline and SetReadDeadline work. They don't wait for a full request/response. Instead they are about single packets.
If you SetReadDeadline of 10 seconds. But the server responds with a single byte every second the timeout will never be hit. But it will take you more than 10 seconds to receive the full response.

The time you pass to DoDeadline can also be different from your ReadTimeout or WriteTimeout.

That's not how SetWriteDeadline and SetReadDeadline work. They don't wait for a full request/response. Instead they are about single packets.
If you SetReadDeadline of 10 seconds. But the server responds with a single byte every second the timeout will never be hit. But it will take you more than 10 seconds to receive the full response.

The time you pass to DoDeadline can also be different from your ReadTimeout or WriteTimeout.

Thank's for your answer. I found that I have miss the loop in HostClient.Do which invokes HostClient.do. But i still found that DoDeadline may timeout before deadline when the request method is POST. When i use a client with ReadTimeout=1s, and now i use client.DoDeadline(req, rsp, 10s) which i want to wait for 10s to receive response. It will wait for 10s only when reqeust method is one of Get/Put/Head, because those methods are retryable.

// default  isRequestRetryable function
// users may set isRequestRetryable through Client.RetryIf
func isIdempotent(req *Request) bool {
	return req.Header.IsGet() || req.Header.IsHead() || req.Header.IsPut()
}

when the request method is POST, it will returns after 1s, because POST is not retryable by default.

[erikdubbelboer]

Actually, I was wrong. Deadlines aren't reset each time a packet arrives.

But I still don't see the issue. The idea is that you can set ReadTimeout and/or WriteTimeout to different values than you pass to clientDoDeadline.

You can for example set:

ReadTimeout = time.Second*4
WriteTimeout = time.Second*4
clientDoDeadline(req, res, time.Second*5)

Which would work for requests that take 2 seconds write and 2 seconds to read. But wouldn't work for requests that take 3 seconds to write and 3 seconds to read.
You can also set ReadTimeout = 60s and do clientDoDeadline(req, res, time.Second*5) in which case the function will return after 5 seconds but the request will keep running in the background for at least 60 seconds (and the result will be discarded).

I'm not sure why you are setting ReadTimeout=1s if you want to wait for 10 seconds?

[chhquan]

@erikdubbelboer Sometimes we will reuse a client for many requests. We will set ReadTimeout=5s, WriteTimeout=5s by default. Once an POST request need longer deadline, it will use Client.DoDeadline instead of client.Do, and set deadline to such as 30 seconds。But the request will still timeout after 5 seconds because clientDoDeadline will not retry for POST.

[erikdubbelboer]

That's not how it works I'm afraid. I also don't like the DoDeadline methods as they are confusing. But I'm afraid we can't remove them for backwards compatibility.

But maybe we should mark them as deprecated and add DoTimeouts(req *Request, resp *Response, readTimeout, writeTimeout time.Duration) which just overwrites the ReadTimeout and WriteTimeout instead of the current weird behavior.

[chhquan]

@erikdubbelboer overwrite ReadTimeout and WriteTimeout is a good idea, it can fix the problem.

[erikdubbelboer]

Just an initial idea: #1096

[chhquan]

@erikdubbelboer Thanks!