🐛 [Bug]: Cache mutex will blocks other requests if any of the cached API is slow.

[harsh-98]

Bug Description

https://github.com/gofiber/fiber/blob/master/middleware/cache/cache.go#L57 The refactored mutex logic in the cache still blocks other requests. The same mutex is used for retrieving cached values and saving the response of cached API. So, if a slow API is cached, it will block the retrieval of cached values for other APIs.

I don't think the mutex is required here. The memory manager itself has rwMutex lock to prevent race conditions. The only behavioral change without mutex is that for the same key, if multiple requests are made simultaneously the response of the last request will override the first request. But since cache implementation only allows saving GET method responses. I don't think saving first response is of importance.

Expected Behavior

Don't block other requests if cached API is slow.

Fiber Version

v2.36.0

Checklist:

• [X] I agree to follow Fiber's Code of Conduct.
• [X] I have checked for existing issues that describe my problem prior to opening this one.
• [X] I understand that improperly formatted bug reports may be closed without explanation.

[welcome[bot]]

Thanks for opening your first issue here! 🎉 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

[ReneWerner87]

https://github.com/gofiber/fiber/commit/b9efc76722f9ff2e0a7d52d9f98116f4eb635674

@apoq can you help here ?

[ReneWerner87]

can you provide a pull request with some tests that will solve the problem

[apoq]

@ReneWerner87 @harsh-98 this been manually tested, after #1764 MR the problematic lock has been fixed, please see below:

// make sure we're not blocking concurrent requests - do unlock
mux.Unlock()

// Continue stack, return err to Fiber if exist
if err := c.Next(); err != nil {
   return err
}

// lock entry back and unlock on finish
mux.Lock()

Also did an automated test with this:

func Test_Cache_Concurrent_Read(t *testing.T) {
	app := fiber.New()
	app.Use(New(Config{Expiration: time.Minute, StoreResponseHeaders: true, CacheHeader: "X-Cache"}))

	app.Get("/fast", func(c *fiber.Ctx) error {
		return c.Status(fiber.StatusOK).SendString("fast")
	})

	app.Get("/slow", func(c *fiber.Ctx) error {
		time.Sleep(time.Millisecond * 300)
		return c.Status(fiber.StatusOK).SendString("slow")
	})
	start := time.Now()

	// first: /fast miss
	rsp, err := app.Test(httptest.NewRequest("GET", "/fast", nil))
	utils.AssertEqual(t, nil, err)
	utils.AssertEqual(t, "miss", rsp.Header.Get("X-Cache"))

	// second: /slow miss cache, check if it blocks further request to /fast
	var slowRespCacheHeaderChan = make(chan string)
	go func(ch chan string) {
		srsp, _ := app.Test(httptest.NewRequest("GET", "/slow", nil), 350)
		ch <- srsp.Header.Get("X-Cache")
	}(slowRespCacheHeaderChan)

	// third: /fast hit, check if it went in less than 10ms (slow one could potentially block it for 300ms)
	rsp, err = app.Test(httptest.NewRequest("GET", "/fast", nil))
	utils.AssertEqual(t, nil, err)
	utils.AssertEqual(t, "hit", rsp.Header.Get("X-Cache"))
	timeSpentForFast := time.Since(start)
	utils.AssertEqual(t, true, timeSpentForFast < time.Millisecond*10)

	slowHeader := <-slowRespCacheHeaderChan
	timeSpentForSlow := time.Since(start)
	utils.AssertEqual(t, true, timeSpentForSlow >= time.Millisecond*300)
	utils.AssertEqual(t, "miss", slowHeader)
}

Good point about redundant use of mutex here, that's true, when actual manager does it on it's own, but there's no mentioned bug here (ie slow routes when cached, do not affect other routes). That was the point of #1764

I'll do some tests later to remove redundant mutex. Thanks