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

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

Open
park-hg opened this issue Sep 3, 2023 · 1 comment · May be fixed by #3035
Labels
apm:ecosystem contrib/* related feature requests or bugs bug unintended behavior that has to be fixed

Comments

@park-hg
Copy link

park-hg commented Sep 3, 2023

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 park-hg added the bug unintended behavior that has to be fixed label Sep 3, 2023
@ahmed-mez ahmed-mez added the apm:ecosystem contrib/* related feature requests or bugs label Sep 19, 2023
@park-hg
Copy link
Author

park-hg commented Nov 1, 2023

@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.

tonyduburque added a commit to tonyduburque/dd-trace-go that referenced this issue Dec 12, 2024
…tion

The Middleware was incorrectly using `c.Context()` instead of `c.UserContext()`
to start spans. This could break the propagation of the context defined by other
middlewares earlier in the chain.

This change ensures that `tracer.StartSpanFromContext` uses the correct context
provided by Fiber, preserving context propagation and ensuring proper tracing behavior.

Fixes DataDog#2199
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:ecosystem contrib/* related feature requests or bugs bug unintended behavior that has to be fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants