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.

Sign up for GitHub

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

Jump to bottom

🐛 Fix expiration time in cache middleware #1881

Merged
merged 3 commits into from May 1, 2022
Merged

🐛 Fix expiration time in cache middleware #1881

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f4415ed
:bug: Fix: Expiration time in cache middleware
breakbuidl Apr 26, 2022
239cb20
:rotating_light: Improve Test_CustomExpiration
breakbuidl Apr 26, 2022
87be43a
- stabilization of the tests
ReneWerner87 May 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 6 additions & 6 deletions middleware/cache/cache.go
View file
Open in desktop
Expand Up @@ -168,23 +168,23 @@ func New(config ...Config) fiber.Handler {
}

// default cache expiration
expiration := uint64(cfg.Expiration.Seconds())
expiration := cfg.Expiration
// Calculate expiration by response header or other setting
if cfg.ExpirationGenerator != nil {
expiration = uint64(cfg.ExpirationGenerator(c, &cfg).Seconds())
expiration = cfg.ExpirationGenerator(c, &cfg)
}
e.exp = ts + expiration
e.exp = ts + uint64(expiration.Seconds())

// For external Storage we store raw body separated
if cfg.Storage != nil {
manager.setRaw(key+"_body", e.body, cfg.Expiration)
manager.setRaw(key+"_body", e.body, expiration)
// avoid body msgp encoding
e.body = nil
manager.set(key, e, cfg.Expiration)
manager.set(key, e, expiration)
manager.release(e)
} else {
// Store entry in memory
manager.set(key, e, cfg.Expiration)
manager.set(key, e, expiration)
}

c.Set(cfg.CacheHeader, cacheMiss)
Expand Down
35 changes: 30 additions & 5 deletions middleware/cache/cache_test.go
View file
Open in desktop
Expand Up @@ -291,15 +291,40 @@ func Test_CustomExpiration(t *testing.T) {
}}))

app.Get("/", func(c *fiber.Ctx) error {
c.Response().Header.Add("Cache-Time", "6000")
return c.SendString("hi")
c.Response().Header.Add("Cache-Time", "2")
now := fmt.Sprintf("%d", time.Now().UnixNano())
return c.SendString(now)
})

req := httptest.NewRequest("GET", "/", nil)
_, err := app.Test(req)
resp, err := app.Test(httptest.NewRequest("GET", "/", nil))
utils.AssertEqual(t, nil, err)
utils.AssertEqual(t, true, called)
utils.AssertEqual(t, 6000, newCacheTime)
utils.AssertEqual(t, 2, newCacheTime)

// Sleep until the cache is expired
time.Sleep(3 * time.Second)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please reduce the time for this, otherwise the test procedure will be greatly extended

you can also wait nanoseconds, micro or milliseconds, whichever is stable

if necessary i would decide for a few milliseconds, because the process also takes a bit of time

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some very strange functioning from code.

I set expiration time to time.Millisecond * time.Duration(newCacheTime). And, when time > 1 sec i.e newCacheTime >= 1000 everything works fine. But, if newCacheTime < 1000 it seems that the response is not cached at all.

   app.Use(New(Config{ExpirationGenerator: func(c *fiber.Ctx, cfg *Config) time.Duration {
   	called = true
   	newCacheTime, _ = strconv.Atoi(c.GetRespHeader("Cache-Time", "600"))
   	return time.Millisecond * time.Duration(newCacheTime)
   }}))

   app.Get("/", func(c *fiber.Ctx) error {
   	c.Response().Header.Add("Cache-Time", "500")
   	now := fmt.Sprintf("%d", time.Now().UnixNano())
   	return c.SendString(now)
   })

   resp, err := app.Test(httptest.NewRequest("GET", "/", nil))
   utils.AssertEqual(t, nil, err)
   utils.AssertEqual(t, true, called)
   utils.AssertEqual(t, 500, newCacheTime)

   // Sleep until the cache is expired
   // time.Sleep(700 * time.Millisecond)

   cachedResp, err := app.Test(httptest.NewRequest("GET", "/", nil))
   utils.AssertEqual(t, nil, err)

   body, err := ioutil.ReadAll(resp.Body)
   utils.AssertEqual(t, nil, err)
   cachedBody, err := ioutil.ReadAll(cachedResp.Body)
   utils.AssertEqual(t, nil, err)

   if !bytes.Equal(body, cachedBody) {
   	t.Errorf("Cache should not have expired: %s, %s", body, cachedBody)
   }

The above test fails with the message - Cache should not have expired: 1651076560555101139, 1651076560555237907

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried setting newCacheTime to 999 and it failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I don't know if it's related or not. But, I played around with Test_CacheExpired and when Expiration is set to less than 1 Second, somehow the expiration time is 1 Minute

The below test fails.

t.Parallel()

	app := fiber.New()
	app.Use(New(Config{Expiration: 500 * time.Millisecond}))

	app.Get("/", func(c *fiber.Ctx) error {
		return c.SendString(fmt.Sprintf("%d", time.Now().UnixNano()))
	})

	resp, err := app.Test(httptest.NewRequest("GET", "/", nil))
	utils.AssertEqual(t, nil, err)
	body, err := ioutil.ReadAll(resp.Body)
	utils.AssertEqual(t, nil, err)

	// Sleep until the cache is expired
	time.Sleep(1000 * time.Millisecond)

	respCached, err := app.Test(httptest.NewRequest("GET", "/", nil))
	utils.AssertEqual(t, nil, err)
	bodyCached, err := ioutil.ReadAll(respCached.Body)
	utils.AssertEqual(t, nil, err)

	if bytes.Equal(body, bodyCached) {
		t.Errorf("Cache should have expired: %s, %s", body, bodyCached)
	}

However, if I change the sleep time to more than 60 seconds say 61000 * time.Millisecond, it passes.


cachedResp, err := app.Test(httptest.NewRequest("GET", "/", nil))
utils.AssertEqual(t, nil, err)

body, err := ioutil.ReadAll(resp.Body)
utils.AssertEqual(t, nil, err)
cachedBody, err := ioutil.ReadAll(cachedResp.Body)
utils.AssertEqual(t, nil, err)

if bytes.Equal(body, cachedBody) {
t.Errorf("Cache should have expired: %s, %s", body, cachedBody)
}

// Next response should be cached
cachedRespNextRound, err := app.Test(httptest.NewRequest("GET", "/", nil))
utils.AssertEqual(t, nil, err)
cachedBodyNextRound, err := ioutil.ReadAll(cachedRespNextRound.Body)
utils.AssertEqual(t, nil, err)

if !bytes.Equal(cachedBodyNextRound, cachedBody) {
t.Errorf("Cache should not have expired: %s, %s", cachedBodyNextRound, cachedBody)
}
}

func Test_AdditionalE2EResponseHeaders(t *testing.T) {
Expand Down