Switching to a different session store leaves memstore goroutine unreleased

[phongphongg]

scs.New() automatically invokes memstore.New(), which creates a goroutine to handle expired session cleanup. Because memstore is the default store, switching to a different store (like gormstore) leaves the memstore goroutine active and unreleased

https://github.com/alexedwards/scs/blob/7e11d57e8885fd38c604d4f79f0eef355650b997/session.go#L106

// Initialize a new session manager and configure it to use gormstore as the session store.
sessionManager = scs.New()
if sessionManager.Store, err = gormstore.New(db); err != nil {
    log.Fatal(err)
}

Workaround solution:

if memStore, ok := sessionManager.Store.(*memstore.MemStore); ok {
    // HACK: Not sure why, but it seems a delay is needed before stopping the cleanup goroutine.
    time.Sleep(time.Second)
    memStore.StopCleanup()
}

if sessionManager.Store, err = gormstore.New(db); err != nil {
    log.Fatal(err)
}

[alexedwards]

You're right, it does. Good spot 👍

I'm not sure what the best course of action here is. Memstore needs to remain the default store to avoid breaking backward compatibility. The cleanup needs to continue to run automatically to avoid breaking backward compatibility. We can't unexport the Store field and require everyone to use a new SetStore() method (which releases the memstore goroutine) without breaking backward compatibility. I don't want to issue a v3 of this package just to resolve this issue.

So I'm a bit stuck. Living with an unreleased goroutine (which won't be using much resources) also isn't ideal but may be the least bad option here. Can you think of any other possibilities?

[dpotapov]

Would be nice to have ability bypass the New() and instantiate the SessionManager without constructor. It is currently impossible because contextKey is private.

[francoposa]

@alexedwards Would you be open to just adding a NewWithOptions (functional options) or NewWithConfig approach from https://github.com/alexedwards/scs/issues/180 ?

Functional options are pretty standard at this point for instantiating things in Go, and avoids creating more of these exact compatibility issues in the future.

[alexedwards]

@dpotapov That's a great suggestion, thank you 👍

I've made ContextKey an exported field in https://github.com/alexedwards/scs/commit/fb8d6f122cfb2228bb787059a8908bb27ecb4cda, which makes it possible to work around this issue by creating your own SessionManager instance, rather than using the New constructor.

cc @phongphongg