Rewrite session cache in OpenSSL ports.

net/ssl/ssl_client_session_cache_openssl.h

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef NET_SSL_SSL_CLIENT_SESSION_CACHE_OPENSSL_H
+#define NET_SSL_SSL_CLIENT_SESSION_CACHE_OPENSSL_H
+
+#include "base/containers/mru_cache.h"
+#include "base/macros.h"
+#include "base/threading/non_thread_safe.h"
+#include "base/time/time.h"
+#include "net/base/net_export.h"
+#include "net/ssl/scoped_openssl_types.h"
+
+namespace net {
+
+class NET_EXPORT SSLClientSessionCacheOpenSSL : public base::NonThreadSafe {

[Ryan Sleevi, 2015/03/17 00:50:33]

grumblenit: I dislike inheritance from traits classes (such as
base::NonThreadSafe), prefering instead to encapsulate traits via composition
(base::ThreadChecker)

[davidben, 2015/03/20 22:41:27]

Done.

+ public:
+  struct Config {
+    Config();

[Ryan Sleevi, 2015/03/17 00:50:34]

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Initialization
recommends in-class member initialization, which is blessed by
https://chromium-cpp.appspot.com/

[davidben, 2015/03/20 22:41:27]

Done, although I'm unclear... the syntax I went with doesn't add a static
initializer, does it? I was kinda surprised it even compiled. You're apparently
allowed to even implicitly reference |this| in these things, so I'm guessing
it's fine?

+
+    // The maximum number of entries in the cache.
+    size_t max_entries;
+    // The number of calls to Lookup before a new check for expired sessions.
+    size_t expiration_check_count;
+    // How long each session should last.
+    base::TimeDelta timeout;
+  };
+
+  explicit SSLClientSessionCacheOpenSSL(const Config& config);
+  ~SSLClientSessionCacheOpenSSL();

[Ryan Sleevi, 2015/03/17 00:50:34]

If you have virtual methods, you should have a virtual dtor.

[davidben, 2015/03/20 22:41:27]

Done.

+
+  size_t size() { return cache_.size(); }

[Ryan Sleevi, 2015/03/17 00:50:34]

1) const
2) Don't inline this; you have no idea what complexity goes on with respect to
instantiating cache_ or calling .size()

[davidben, 2015/03/20 22:41:27]

Done.

+
+  // Returns the entry associated with |cache_key| or null if there is none. The
+  // caller is responsible for taking a reference to the pointer if the cache is
+  // destroyed or a call to Insert is made.
+  SSL_SESSION* Lookup(const std::string& cache_key);

[Ryan Sleevi, 2015/03/17 00:50:34]

const? non-const?

[davidben, 2015/03/20 22:41:27]

Non-const. It updates the MRU. base::MRUCache is the same. I've added a comment
about it.

+
+  // Inserts |session| into the cache at |cache_key|. If there is an existing
+  // one, it is released. Every |expiration_check_count| calls, the cache is
+  // checked for stale entries.
+  void Insert(const std::string& cache_key, SSL_SESSION* session);
+
+  // Removes all entries from the cache.
+  void Flush();
+
+ protected:
+  // Overridable base::Time::Now wrapper for testing.
+  virtual base::Time Now();

[Ryan Sleevi, 2015/03/17 00:50:34]

Rather than inheritance for testing, why not use a base::Clock / base::TickClock
for this, passed in construction?

[davidben, 2015/03/20 22:41:27]

Done.

+
+ private:
+  struct CacheEntry {
+    CacheEntry();
+    ~CacheEntry();
+
+    ScopedSSL_SESSION session;
+    // The time at which this entry expires.
+    base::Time expiration;

[Ryan Sleevi, 2015/03/17 00:50:34]

base::TimeTicks, I would presume; otherwise, if the user adjusts their clock,
everything gets ruined here.

[davidben, 2015/03/20 22:41:27]

I used that initially, but I think it actually doesn't want to be TimeTicks. If
you suspend the machine for an hour and bring it back, will TimeTicks fast
forward for an hour? I don't think it does, but you still want to avoid reusing
an expired session. IOW, the expiration time is about how much longer, in wall
clock time, will I continue using this key.

(The old code used the wall clock time. Actually it relied on OpenSSL to stuff
the time in with time(), so it wasn't even mockable.)

+  };
+  using CacheEntryMap =

[Ryan Sleevi, 2015/03/17 00:50:34]

newline

[davidben, 2015/03/20 22:41:27]

Done.

+      base::MRUCacheBase<std::string,
+                         CacheEntry*,
+                         base::MRUCachePointerDeletor<CacheEntry*>,
+                         base::MRUCacheHashMap>;
+
+  // Removes all expired sessions from the cache.
+  void FlushExpiredSessions();
+
+  Config config_;
+  CacheEntryMap cache_;
+  size_t lookups_since_flush_;
+
+  DISALLOW_COPY_AND_ASSIGN(SSLClientSessionCacheOpenSSL);
+};
+
+}  // namespace net
+
+#endif  // NET_SSL_SSL_CLIENT_SESSION_CACHE_OPENSSL_H