OrchardCore.Commerce icon indicating copy to clipboard operation
OrchardCore.Commerce copied to clipboard

Cart vanishes when the shell is restarted (OCC-276)

Open Piedone opened this issue 1 year ago • 5 comments

Describe the bug

If you have items in the cart, a shell restart will wipe it out. This is an issue because configuration changes, deployments can cause the shell to restart at any time, but this disrupts shopping and causes the shop to lose money.

To Reproduce

  1. Launch the latest main (9d3d6859b15672c28c55825d6fbca4b495cad97e).
  2. Add something to the cart.
  3. Restart the shell/tenant (not necessarily the app, but that's also relevant). E.g., enable any feature. For the sake of simplicity, I enabled the harmless Background Tasks one.
  4. Observe that after a refresh on the frontend, the contents of the cart are gone.

Expected behavior

The cart remains after a shell restart too.

Screenshots

https://github.com/user-attachments/assets/58e46923-4269-4933-898d-f56364385c1a

Jira issue

Piedone avatar Aug 01 '24 21:08 Piedone

This is not a bug. The reference implementation of IShoppingCartPersistence is SessionShoppingCartPersistence. This serializes the cart contents into HttpContext.Session, which is volatile storage that's lost when the application closes.

The advantage of this temporary storage is both ease of implementation and not having to worry about invalidating and periodically cleaning up abandoned carts (otherwise abandoned carts could accidentally or maliciously eat up the site's database space). The disadvantage is what you described. However config changes and deployments shouldn't happen so often where this becomes a realistic problem in everyday use, so session storage remains a suitable default.

Web developers using OCC can implement their own IShoppingCartPersistence which uses site setting, an external database, etc for persistent storage. A purely client side storage is also an option. We may add a different implementation as a feature in the future. The added complication and the unclear demand makes me less convinced that this should be a priority. But if desired, please convert this from bug to a feature issue.

sarahelsaig avatar Aug 01 '24 21:08 sarahelsaig

Really every user-facing feature needs to be resilient so shell restarts in an OC app (as it is with the OOTB features, like user logins remain, and you can edit content items without disruption), since a shell restart is not at all uncommon. It happens e.g. in the following scenarios:

  • A lot of site settings and other configuration changes restart the shell apart from turning features on or off.
  • On DotNest, we shut down idle tenants. If you start shopping, get up to do whatever, then come back later to finish it, you might hit a now newly starting shell.
  • In an actively developed app deployments and prod rollouts can happen daily, or even multiple times a day. (There are periods like this in our apps too.)

And furthermore, horizontal scaling should be possible to support as well.

I don't think the current implementation is really appropriate, then.

Piedone avatar Aug 01 '24 21:08 Piedone

Can we just enable OrchardCore.Redis.Cache feature to overcome this issue?

infofromca avatar Mar 03 '25 15:03 infofromca

Just enabling the feature is not enough. It implements IDistributedCache which is not currently used for cart persistence. But IDistributedCache may be a could candidate for rewriting SessionShoppingCartPersistence, then it would use MemoryDistributedCache by default but switch to RedisCacheWrapper if the Redis feature is enabled. Something to look into.

sarahelsaig avatar Mar 05 '25 09:03 sarahelsaig

This is what I got from Grok: Here’s how it ties together:

Microsoft.AspNetCore.Session: This package enables session state in ASP.NET Core. By default, it stores session data in-memory using IDistributedCache. It doesn’t inherently connect to Redis—it relies on whatever IDistributedCache implementation you configure. Microsoft.Extensions.Caching.StackExchangeRedis: This package provides a Redis-based implementation of IDistributedCache. You’d use this to store session data in Redis instead of in-memory.

canadacubachina avatar Mar 05 '25 16:03 canadacubachina