Improve locking in TLS::Session_Manager

[reneme]

Following the feedback of @oviano I had another look at the mutex usage in Session_Manager and Session_Manager_SQL(ite).

The base class is now taking hold of the mutex only if absolutely necessary (rationales as comments in the code). This is meant to get out of the way of application-defined session managers that try to optimize for throughput.

The locking in Session_Manager_SQLite is reduced if the underlying SQLite library was compiled with mutex support. Note that the SQLite adapter leaves quite a bit more room for optimization. This could be tackled independently. Like:

• efficiently use prepared statements
• think about using manual transactions
• allow applications to manage thread-local SQLite session managers to take better advantage of SQLite's "multi-threaded" mode.

[randombit]

Still a draft I know but looks good so far, pre-approving so you can merge when completed.

[oviano]

Looks great.

One thing is that we'd need a way of controlling the open_flags in the sqlite3 engine.

For example, let's say I was going to have multiple Session_Manager_SQLite on a thread-local basis, (or, I was going to derive a new Session_Manager_SQLite_TLDB class which internally managed multiple DB thread-local connections), then I would need to be able to choose the open flag SQLITE_OPEN_NOMUTEX for each connection so that there is no unnecessary locking going on (I'm not sure of the overhead, but better to reduce locking options where we can).

So maybe we can either allow passing the open_flags directly into the engine via the session manager constructor, or if you want to control it then have a new enum specifying behaviour we want, and map to the appropriate open_flags.

[reneme]

Please see the latest two commits. This adds an optional parameter to Database_SQLite where one can pass arbitrary open flags. Additionally, the locking decision can now be overridden with Session_Manager_SQL::database_is_threadsafe().

With this you should have all the things to build your own flavor of an SQLite session manager as you sketched before. Basically, by deriving from Session_Manager_SQL:

class Multithreaded_Session_Manager_SQLite final : public Session_Manager_SQL
   {
   public:
      Multithreaded_Session_Manager_SQLite(const std::string& passphrase,
                                           std::shared_ptr<RandomNumberGenerator> rng,
                                           const std::string& db_filename,
                                           size_t max_sessions = 1000)
         : Session_Manager_SQL(std::make_shared<Sqlite3_Database>(db_filename, SQLITE_OPEN_READWRITE |
                                                                               SQLITE_OPEN_CREATE    |
                                                                               SQLITE_OPEN_NOMUTEX),
                               passphrase,
                               rng,
                               max_sessions) {}

      bool database_is_threadsafe() const override { return true; }
   };

Frankly, I'm hesitant to add an option to the off-the-shelf Session_Manager_SQLite enabling this behavior. Simply because it feels like a fairly special use case that might cause unnecessary confusion to users that don't need that.

I'd be curious about your benchmarking results. Both regarding the influence on this locking and the TLS performance in general.

[oviano]

That's perfect, thank you very much!

[randombit]

👍