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

chore(context): http.CloseNotify is deprecated, use context.Done instead #3994

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

0xff-dev
Copy link

http.CloseNotify is deprecated, use context.Done instead.

@appleboy appleboy added this to the v1.11 milestone Jun 15, 2024
@appleboy appleboy changed the title chore: http.CloseNotify is deprecated, use context.Done instead chore(context): http.CloseNotify is deprecated, use context.Done instead Jun 15, 2024
@appleboy
Copy link
Member

@0xff-dev Please fix the build error.

@0xff-dev
Copy link
Author

@0xff-dev Please fix the build error.

Done.
After checking the action log, I found that the closeClient function was not used after the code was changed. The closeClient function has been deleted.

@lwabish
Copy link

lwabish commented Oct 14, 2024

Sorry to distract a little bit from this mr but I have a related problem to consult:

Consider following code, when I stop my request from client side with ctrl+c, why c.Request.Context.Done doesn't work but c.Writer.CloseNotify() does?

func(c *gin.Context) {
    done := make(chan struct{})
    
    go func() {
	    time.Sleep(5 * time.Second)
	    close(done)
    }()
    
    select {
    case <-done:
	    c.JSON(200, gin.H{"message": "done"})
    case <-c.Writer.CloseNotify():
	    logrus.Warningf("close writer close done")
	    c.JSON(499, gin.H{"error": "close writer close done"})
    }
}

@0xff-dev
Copy link
Author

Sorry to distract a little bit from this mr but I have a related problem to consult:

Consider following code, when I stop my request from client side with ctrl+c, why c.Request.Context.Done doesn't work but c.Writer.CloseNotify() does?

func(c *gin.Context) {
    done := make(chan struct{})
    
    go func() {
	    time.Sleep(5 * time.Second)
	    close(done)
    }()
    
    select {
    case <-done:
	    c.JSON(200, gin.H{"message": "done"})
    case <-c.Writer.CloseNotify():
	    logrus.Warningf("close writer close done")
	    c.JSON(499, gin.H{"error": "close writer close done"})
    }
}

Hi @lwabish , what version of go are you using? I've tested it and both methods can cancel the request normally, go version is 1.23.2 .

// server
func ok(ctx *gin.Context) {
	fmt.Println("call ok")
	done := make(chan struct{})
	go func() {
		time.Sleep(5 * time.Second)
		fmt.Println("after 5s, task done")
		close(done)
	}()

	select {
	case <-done:
		ctx.JSON(200, gin.H{"message": "done"})
	case <-ctx.Writer.CloseNotify():
		ctx.JSON(499, gin.H{"err": "close write close"})
		/*
			case <-ctx.Request.Context().Done():
				fmt.Println("context deadline")
				ctx.JSON(499, gin.H{"err": "close write close"})
		*/
	}
}

func main() {
	e := gin.Default()
	e.GET("/hello", ok)

	e.Run(":8000")
}


// client

import (
	"fmt"
	"net/http"
)

func main() {
	resp, _ := http.Get("http://localhost:8000/hello")
	fmt.Printf("resp: %v\n", *resp)
}
image

@lwabish
Copy link

lwabish commented Oct 14, 2024

@0xff-dev

A lot of thanks for your timely response. I did some experiment with your code and indeed both method worked as expected.

Focus on my code, with go version 1.23, too, the handler seems nothing different from yours so I have moved my sight to how http server ran. I found my code is not using gin.Default() and e.Run() but have some customization of how http.Server runs. Maybe this is where magic happened? I'll dig more later.

FYI, this is how the http server is built in my case:

Server: &http.Server{
                        // in.Engine is from gin.New()
			Handler:     handlers.CORS()(in.Engine),
		}

And this is how the server is run:

go func() {
		err := svr.ListenAndServe()
		if errors.Is(err, http.ErrServerClosed) {
			logger.Infof("server closed")
		} else {
			logger.Errorf("stop server err: %s", err)
		}
	}()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants