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

🐛 Fix expiration time in cache middleware #1881

Merged

Conversation

breakbuidl
Copy link
Contributor

@breakbuidl breakbuidl commented Apr 26, 2022

Even if ExpirationGenerator was provided in cache config, Expiration was used anyway.

fixes #1869

* Custom expiration time using ExpirationGenerator is also functional
now instead of default Expiration only
@welcome
Copy link

welcome bot commented Apr 26, 2022

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@breakbuidl breakbuidl force-pushed the fix-cache-expiration-generator branch from ca034be to 239cb20 Compare Apr 26, 2022
@breakbuidl
Copy link
Contributor Author

breakbuidl commented Apr 27, 2022

Could any of the maintainers please approve running workflows?

utils.AssertEqual(t, 2, newCacheTime)

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

@ReneWerner87 ReneWerner87 Apr 27, 2022

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

@breakbuidl breakbuidl Apr 27, 2022

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

@breakbuidl breakbuidl Apr 27, 2022

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

@breakbuidl breakbuidl Apr 27, 2022

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.

- speed up the cache tests
- fix race conditions in client and client tests
@ReneWerner87 ReneWerner87 merged commit bb9ac8f into gofiber:master May 1, 2022
16 checks passed
@welcome
Copy link

welcome bot commented May 1, 2022

Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

trim21 pushed a commit to trim21/fiber that referenced this issue Aug 15, 2022
* 🐛 Fix: Expiration time in cache middleware

* Custom expiration time using ExpirationGenerator is also functional
now instead of default Expiration only

* 🚨 Improve Test_CustomExpiration

* - stabilization of the tests
- speed up the cache tests
- fix race conditions in client and client tests

Co-authored-by: wernerr <rene@gofiber.io>
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