DataDog / dd-trace-go

Datadog Go Library including APM tracing, profiling, and security monitoring.
https://docs.datadoghq.com/tracing/
Other
674 stars 438 forks source link

[BUG] fiber app may not shutdown gracefully when using dd-trace-go/contrib/gofiber/fiber.v2/fiber.go middleware #2199

Open park-hg opened 1 year ago

park-hg commented 1 year ago

I'm not sure this is a bug or intended.

Version of dd-trace-go

1.56.1

Describe what happened:

after calling Middleware function, c.UserContext() returns *fasthttp.RequestCtx which carries a deadline a cancellation signal.(https://github.com/gofiber/fiber/blob/master/ctx.go#L443)

Describe what you expected: Middleware function should not mutate the returned context of c.UserContext(), which means c.UserContext() should not have any deadline or cancellation signals if not set previously.

Steps to reproduce the issue:

suppose there are 2 servers; s1(localhost:3000), s2(localhost:8080), and s1 sends GET request to s2. s1 needs to shutdown gracefully. s1 uses Middleware function as below.

server 1 (s1)

package main

import (
    "fmt"
    "io"
    "log"
    "net/http"
    "os"
    "os/signal"
    "syscall"

    "github.com/gofiber/fiber/v2"
)

var exitChannel = make(chan os.Signal, 1)
var shutdownChannel = make(chan struct{}, 1)

func main() {
    app := fiber.New()

    app.Use(fiber2.Middleware())
    app.Get("/", func(c *fiber.Ctx) error {
        client := http.Client{
            Transport: http.DefaultTransport,
        }
        req, err := http.NewRequestWithContext(c.UserContext(), "GET", "http://localhost:8080", nil)
        if err != nil {
            return err
        }
        resp, err := client.Do(req)
        if err != nil {
            return err
        }
        defer resp.Body.Close()
        bytes, err := io.ReadAll(resp.Body)
        if err != nil {
            return err
        }
        return c.Status(fiber.StatusOK).SendString(string(bytes))
    })

    signal.Notify(exitChannel, syscall.SIGINT, syscall.SIGTERM)
    go func() {
        _ = <-exitChannel
        fmt.Println("gracefully shutting down...")
        _ = app.Shutdown()
        close(shutdownChannel)
    }()
    if err := app.Listen(":3000"); err != nil {
        log.Panic(err)
    }

    <-shutdownChannel
    fmt.Println("running cleanup tasks...")
    // teardown
}

server 2 (s2)

package main

import (
    "log"
    "time"

    "github.com/gofiber/fiber/v2"
)

func main() {
    app := fiber.New()

    app.Get("/", func(c *fiber.Ctx) error {
        time.Sleep(5 * time.Second)

        return c.SendString("hello world!")
    })

    log.Fatal(app.Listen(":8080"))
}

when sending SIGTERM to s1 during s1 is handling requests, s1 shutdowns immediately, which does not when s1 without Middleware function.

Additional environment details (Version of Go, Operating System, etc.): Go 1.21.x

Proposal use c.UserContext()(https://github.com/gofiber/fiber/blob/master/ctx.go#L451) instead of c.Context() (type *fasthttp.RequestCtx) when start span as below.

// dd-trace-go/contrib/gofiber/fiber.v2/fiber.go#L61
span, ctx := tracer.StartSpanFromContext(c.Context(), cfg.spanName, opts...)

This matches with other framework's Middleware function...

park-hg commented 1 year ago

@ahmed-mez hello, this is a remind for this issue. I edited the comment and it's my pleasure if you give me some reviews for this issue. if this seems like not a bug, let me close it.