Skip to content

Conversation

@demouth
Copy link
Contributor

@demouth demouth commented Aug 22, 2023

Overview

Currently, when a timeout occurs, the ctx.Request.Context().Err() method returns a context.Canceled. However, in the case of a timeout, we expect the context.DeadlineExceeded to be returned.

This pull request addresses the timeout error handling. By modifying to return context.DeadlineExceeded when a timeout occurs, we enhance the consistency of error handling.

r := gin.New()
r.GET("/", timeout.New(
	timeout.WithTimeout(50*time.Millisecond),
	timeout.WithHandler(func(c *gin.Context) {
		time.Sleep(100 * time.Millisecond)

		// expected: context.DeadlineExceeded
		// actual: context.Canceled
		fmt.Printf("%v\n", c.Request.Context().Err())
	}),
))
r.Run("0.0.0.0:8083")

Changes Made

Reviewed and revised timeout error handling to return the context.DeadlineExceeded instead of the context.Canceled using ctx.Request.Context().Err().

Testing

Added test cases that trigger timeouts with the applied changes. This ensures that the modifications function correctly and the expected error is returned.

@appleboy appleboy changed the title (context.Context).Err() returns DeadlineExceeded fix(context): (context.Context).Err() returns DeadlineExceeded Oct 7, 2023
@appleboy appleboy merged commit 9e7d799 into gin-contrib:master Oct 7, 2023
appleboy added a commit that referenced this pull request Nov 25, 2023
)

- Remove the import statement for "context" in timeout.go
- Remove the code block related to context cancellation in the New function in timeout.go
- Replace the case statement in the New function in timeout.go with a case statement using time.After
- Remove the import statement for "sync" in timeout_test.go
- Remove the TestDeadlineExceeded function in timeout_test.go

reverted PR #54 

Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
@appleboy
Copy link
Member

@demouth I reverted the PR. Can't pass the testing.

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

Successfully merging this pull request may close these issues.

2 participants