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

SessionManager: Allow backend storage to be configurable #481

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

SessionManager: Allow backend storage to be configurable #481

wants to merge 2 commits into from

Conversation

zachhilman
Copy link
Contributor

This allows using the built-in session feature without having to use the CacheMap, which is limited to the current process and not suitable in larger deployments.

In my use case of drogon, I am running multiple pods behind a single load-balancer, so the built-in session handler won't work without sticky load balancing, which isn't feasible and is more of a bandaid fix. This allows the user to optionally specify their own implementation of backing storage, for example using redis. By default, CacheMap is used and current usages should require no changes, it is opt-in only.

This allows using the built-in session feature without having to use the CacheMap, which is limited to the current process and not suitable in larger deployments.
@an-tao an-tao self-requested a review June 17, 2020 00:49
@an-tao
Copy link
Member

an-tao commented Jun 17, 2020

@DarkLordZach Thanks so much for the patch. this is very helpful for multiple pods scenarios.
I have a quastion, I used synchronous methods in the old session manager because data is stored in memory, the read/write on sessions dosen't block the current thread. but for a custom session storage (e.g. a redis implementation) the calling on synchronous interfaces may block current thread, this will cause a large throughput loss in high concurrency scenarios. So is it necessary to change the synchronous interface to the asynchronous interface?

@an-tao an-tao requested a review from rbugajewski June 17, 2020 02:38
Copy link
Collaborator

@rbugajewski rbugajewski left a comment

Choose a reason for hiding this comment

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

@DarkLordZach Thanks so much for your pull request. Besides the same question @an-tao already asked, which I would like to clarify before this gets merged into master, this changeset LGTM.

I currently don’t have a Redis instance running outside of production environments, and would’ve to set up an environment to test the framework behavior on this branch by myself.

@@ -547,6 +557,7 @@ class HttpAppFrameworkImpl : public HttpAppFramework
size_t clientMaxWebSocketMessageSize_{128 * 1024};
std::string homePageFile_{"index.html"};
std::function<void()> termSignalHandler_{[]() { app().quit(); }};
std::function<std::unique_ptr<SessionStorageProvider>(trantor::EventLoop*, size_t)> sessionStorageCreator_{createCacheMapProvider};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
std::function<std::unique_ptr<SessionStorageProvider>(trantor::EventLoop*, size_t)> sessionStorageCreator_{createCacheMapProvider};
std::function<std::unique_ptr<SessionStorageProvider>(
trantor::EventLoop*, size_t)> sessionStorageCreator_{createCacheMapProvider};

if (!provider_->findAndFetch(sessionID, sessionPtr))
{
sessionPtr = std::make_shared<Session>(sessionID, needToSet);
provider_->insert(sessionID, sessionPtr, timeout_);
Copy link
Member

@an-tao an-tao Jun 17, 2020

Choose a reason for hiding this comment

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

If the provider_ is created for redis, how do we store the sessionPtr to the redis server?
And as the session object (sessionPtr) can be accessed by users via Http Request objects, how do we synchronize its changes to the redis server?

Copy link
Member

@an-tao an-tao Jun 17, 2020

Choose a reason for hiding this comment

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

I think we have to make the Session class as a pure virtual class for different implementations of memory storage or redis storage. But how to deal with the template and the any?

Copy link

Choose a reason for hiding this comment

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

Protobuf for session fields serialization/deserialization?

@zachhilman
Copy link
Contributor Author

@an-tao , Yes, using an asynchronous interface with std::future does seem much better, I didn't do that because I was trying to change as little code as possible. I can fix that today.
My other question was if it was possible to switch from std::any to std::variant<...>, because std::variant is so much easier to handle for serialization/deserialization. I was thinking it could include common types like bool, std::string, size_t, and perhaps even Json::Value. I do realize that this is probably a breaking change though.
Finally, for the sessionPtr stuff you mentioned, yes, I do think it needs to be virtualized, but it should require minimal effort. All that needs to happen is marking the methods as virtual. Since the ptr is returned from SessionStorageProvider, the user already has control over which implementation is used. As for how sessionPtr will connect to redis, I was planning on using RAII, where the session maintains a reference/shared-ptr to the redis client and gets the value in the constructor and then sets it in the destructor.

@an-tao
Copy link
Member

an-tao commented Jun 17, 2020

@DarkLordZach Thanks for your reponse.
If you use std::future, the futureobj.get() is a new blocking point to the current thread, I think maybe we should use callbacks to get session data.
As you said, the changes you mentioned will break the current APIs, so we need more discussion about them. You can start to perfect this PR after we have fully discussed.
@rbugajewski @vedranmiletic @ihmc3jn09hk

@an-tao
Copy link
Member

an-tao commented Jun 17, 2020

@DarkLordZach If we use std::variant instead of std::any, does that mean we can only store data of some predefined types to sessions? This seems to increase the constraints on the type of data stored in sessions.

@zachhilman
Copy link
Contributor Author

Yes, but the problem with std::any is that the only way to get objects out of it is to use std::any_cast, which only works if you use the explicit type. Thus, if the backend cannot directly store the std::any (for example, on redis), it either has to explicitly check for every type the user might store or some other type has to be used.

@an-tao
Copy link
Member

an-tao commented Jun 17, 2020

Yes, but the problem with std::any is that the only way to get objects out of it is to use std::any_cast, which only works if you use the explicit type. Thus, if the backend cannot directly store the std::any (for example, on redis), it either has to explicitly check for every type the user might store or some other type has to be used.

You are right, the current API only suitable for memory storage.

@rbugajewski
Copy link
Collaborator

At this point of development API breaking changes should be prioritized lower in my opinion, so that it would be OK to change the API. But I think we should find a better solution and strive towards getting the best one for a plain memory storage and key-value stores like Redis at the same time, not find a compromise that will hinder the design and usage of one or the other. The current API is beneficial for plain memory storage as it is as flexible as possible. Changing the API to std::variant won’t be a big deal API wise, but it will make the plain memory storage less versatile. On the other hand a configurable key-value store in the backend is a rather common use case and it would be great to have it as a core part of the framework.

I wonder how often you’ve a case that it is necessary to provide a “drop-in” replacement for plain memory storage? From my experience it is rather clearly defined what gets stored in a session and what in a key-value store. In this case we could maybe decouple both APIs, keep std::any for plain memory storage, and use std::variant for Redis? Do you know how the situation looks like in Amazon’s DynamoDB or Memcached?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants