Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

random fail response from server #261

Closed
smthpickboy opened this issue May 26, 2017 · 3 comments
Closed

random fail response from server #261

smthpickboy opened this issue May 26, 2017 · 3 comments

Comments

@smthpickboy
Copy link

smthpickboy commented May 26, 2017

I'm using fasthttp + fasthttprouter.

Go 1.8.1
fasthttp version: b154429
fasthttprouter version: ade4e2031af3aed7fffd241084aad80a58faf421

After I update fasthttp from some old version to the current version, I get random failure response from server.

curl: (52) Empty reply from server
curl: (56) Failure when receiving data from the peer

fasthttprouter hasn't been updated for quite some time.

@smthpickboy
Copy link
Author

smthpickboy commented Jun 2, 2017

After further testing, I found that it's caused by ReadTimeout/WriteTimeout of fasthttp.Server

func main() {
	m := func(ctx *fasthttp.RequestCtx) {
		switch string(ctx.Path()) {
		case "/foo":
			foo(ctx)
		default:
			ctx.Error("not found", fasthttp.StatusNotFound)
		}
	}

	s := fasthttp.Server{
		Handler: m,
		ReadTimeout:  time.Second + 500*time.Millisecond,
		//WriteTimeout: time.Second,
		MaxRequestBodySize: 2 * 1024,
	}

	fmt.Println("testing with ReadTimeout 1.5s")
	err := s.ListenAndServe("127.0.0.1:8000")
	if err != nil {
		fmt.Println(err)
	}
}

func foo(ctx *fasthttp.RequestCtx) {
	ctx.WriteString("Success")
}

If I set ReadTimeout to 1.5 second on my test machine, It's no problem. But if set to less than 1.5 sec, such as 1 sec, there would be some random fail response.

Is there any additional config that I should do to use ReadTimeout/WriteTimeout of fasthttp.Server?

@algorithm-buaa
Copy link

algorithm-buaa commented Mar 2, 2018

func (c *HostClient) doNonNilReqResp(req *Request, resp *Response) (bool, error) {
	if req == nil {
		panic("BUG: req cannot be nil")
	}
	if resp == nil {
		panic("BUG: resp cannot be nil")
	}

	atomic.StoreUint32(&c.lastUseTime, uint32(CoarseTimeNow().Unix()-startTimeUnix))

	// Free up resources occupied by response before sending the request,
	// so the GC may reclaim these resources (e.g. response body).
	resp.Reset()

	cc, err := c.acquireConn()
	if err != nil {
		return false, err
	}
	conn := cc.c

	if c.WriteTimeout > 0 {
		// Optimization: update write deadline only if more than 25%
		// of the last write deadline exceeded.
		// See https://github.com/golang/go/issues/15133 for details.
		currentTime := CoarseTimeNow()
		if currentTime.Sub(cc.lastWriteDeadlineTime) > (c.WriteTimeout >> 2) {
			if err = conn.SetWriteDeadline(currentTime.Add(c.WriteTimeout)); err != nil {
				c.closeConn(cc)
				return true, err
			}
			cc.lastWriteDeadlineTime = currentTime
		}
	}

The op currentTime := CoarseTimeNow() does not get exact current time :


// CoarseTimeNow returns the current time truncated to the nearest second.
//
// This is a faster alternative to time.Now().
func CoarseTimeNow() time.Time {
	tp := coarseTime.Load().(*time.Time)
	return *tp
}

currentTime := time.Now() will be Ok, but I dont kown why to change like that. Maybe for performance.

@erikdubbelboer
Copy link
Collaborator

That’s why I removed coarse time from my fork: erikdubbelboer@05f8f96 (which contains many other fixes as well)

erikdubbelboer added a commit to erikdubbelboer/fasthttp that referenced this issue Aug 17, 2018
It is not clear why @valyala introduced this coarse time. Benchmarks on
different systems show that the speedup is no where enough to justify
the added code complexity and bugs it seems to have introduced.

Mac:
BenchmarkCoarseTimeNow-8   	2000000000	         2.49 ns/op
0 B/op	       0 allocs/op
BenchmarkTimeNow-8         	500000000	         3.14 ns/op
0 B/op	       0 allocs/op

Ubuntu:
BenchmarkCoarseTimeNow-4   	300000000	         6.74 ns/op
0 B/op	       0 allocs/op
BenchmarkTimeNow-4         	100000000	        15.9 ns/op
0 B/op	       0 allocs/op

This reverts commit 6309f42
and 32c72cd.

See: valyala#271,
valyala#269 and
valyala#261.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants