changeset 407:9f17abb48a34

6918573: sun.security.pkcs11.P11RSACipher.finalize() is a scalability blocker Summary: Removed the finalize() methods and use PhantomReference in Session to do auto clean up. Reviewed-by: wetmore
author valeriep
date Fri, 06 Aug 2010 14:56:01 -0700
parents 80a618f36d00
children 3d4d8bc2ef64
files src/share/classes/sun/security/pkcs11/P11Cipher.java src/share/classes/sun/security/pkcs11/P11Digest.java src/share/classes/sun/security/pkcs11/P11Key.java src/share/classes/sun/security/pkcs11/P11Mac.java src/share/classes/sun/security/pkcs11/P11RSACipher.java src/share/classes/sun/security/pkcs11/P11Signature.java src/share/classes/sun/security/pkcs11/Session.java src/share/classes/sun/security/pkcs11/SessionManager.java
diffstat 8 files changed, 181 insertions(+), 131 deletions(-) [+]
line wrap: on
line diff
--- a/src/share/classes/sun/security/pkcs11/P11Cipher.java	Wed Aug 04 00:07:49 2010 +0100
+++ b/src/share/classes/sun/security/pkcs11/P11Cipher.java	Fri Aug 06 14:56:01 2010 -0700
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2007, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2010, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -127,7 +127,6 @@
             // XXX change default to PKCS5Padding
             paddingType = PAD_NONE;
         }
-        session = token.getOpSession();
     }
 
     protected void engineSetMode(String mode) throws NoSuchAlgorithmException {
@@ -654,16 +653,4 @@
                                 (token, key, keyAlgorithm).keyLength();
         return n;
     }
-
-    protected void finalize() throws Throwable {
-        try {
-            if ((session != null) && token.isValid()) {
-                cancelOperation();
-                session = token.releaseSession(session);
-            }
-        } finally {
-            super.finalize();
-        }
-    }
-
 }
--- a/src/share/classes/sun/security/pkcs11/P11Digest.java	Wed Aug 04 00:07:49 2010 +0100
+++ b/src/share/classes/sun/security/pkcs11/P11Digest.java	Fri Aug 06 14:56:01 2010 -0700
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2005, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2010, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -308,16 +308,4 @@
             throw new ProviderException("update() failed", e);
         }
     }
-
-    protected void finalize() throws Throwable {
-        try {
-            if ((session != null) && token.isValid()) {
-                cancelOperation();
-                session = token.releaseSession(session);
-            }
-        } finally {
-            super.finalize();
-        }
-    }
-
 }
--- a/src/share/classes/sun/security/pkcs11/P11Key.java	Wed Aug 04 00:07:49 2010 +0100
+++ b/src/share/classes/sun/security/pkcs11/P11Key.java	Fri Aug 06 14:56:01 2010 -0700
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2006, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2010, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -26,6 +26,7 @@
 package sun.security.pkcs11;
 
 import java.io.*;
+import java.lang.ref.*;
 import java.math.BigInteger;
 import java.util.*;
 
@@ -69,9 +70,6 @@
     // type of key, one of (PUBLIC, PRIVATE, SECRET)
     final String type;
 
-    // session in which the key was created, relevant for session objects
-    final Session session;
-
     // token instance
     final Token token;
 
@@ -87,10 +85,12 @@
     // flags indicating whether the key is a token object, sensitive, extractable
     final boolean tokenObject, sensitive, extractable;
 
+    // phantom reference notification clean up for session keys
+    private final SessionKeyRef sessionKeyRef;
+
     P11Key(String type, Session session, long keyID, String algorithm,
             int keyLength, CK_ATTRIBUTE[] attributes) {
         this.type = type;
-        this.session = session;
         this.token = session.token;
         this.keyID = keyID;
         this.algorithm = algorithm;
@@ -113,7 +113,9 @@
         this.sensitive = sensitive;
         this.extractable = extractable;
         if (tokenObject == false) {
-            session.addObject();
+            sessionKeyRef = new SessionKeyRef(this, keyID, session);
+        } else {
+            sessionKeyRef = null;
         }
     }
 
@@ -238,24 +240,6 @@
         }
     }
 
-    protected void finalize() throws Throwable {
-        if (tokenObject || (token.isValid() == false)) {
-            super.finalize();
-            return;
-        }
-        Session newSession = null;
-        try {
-            newSession = token.getOpSession();
-            token.p11.C_DestroyObject(newSession.id(), keyID);
-        } catch (PKCS11Exception e) {
-            // ignore
-        } finally {
-            token.releaseSession(newSession);
-            session.removeObject();
-            super.finalize();
-        }
-    }
-
     private final static CK_ATTRIBUTE[] A0 = new CK_ATTRIBUTE[0];
 
     private static CK_ATTRIBUTE[] getAttributes(Session session, long keyID,
@@ -1065,5 +1049,68 @@
                 + "\n  parameters: " + params;
         }
     }
+}
 
+/*
+ * NOTE: Must use PhantomReference here and not WeakReference
+ * otherwise the key maybe cleared before other objects which
+ * still use these keys during finalization such as SSLSocket.
+ */
+final class SessionKeyRef extends PhantomReference<P11Key>
+    implements Comparable<SessionKeyRef> {
+    private static ReferenceQueue<P11Key> refQueue =
+        new ReferenceQueue<P11Key>();
+    private static Set<SessionKeyRef> refList =
+        Collections.synchronizedSortedSet(new TreeSet<SessionKeyRef>());
+
+    static ReferenceQueue<P11Key> referenceQueue() {
+        return refQueue;
+    }
+
+    private static void drainRefQueueBounded() {
+        while (true) {
+            SessionKeyRef next = (SessionKeyRef) refQueue.poll();
+            if (next == null) break;
+            next.dispose();
+        }
+    }
+
+    // handle to the native key
+    private long keyID;
+    private Session session;
+
+    SessionKeyRef(P11Key key , long keyID, Session session) {
+        super(key, refQueue);
+        this.keyID = keyID;
+        this.session = session;
+        this.session.addObject();
+        refList.add(this);
+        // TBD: run at some interval and not every time?
+        drainRefQueueBounded();
+    }
+
+    private void dispose() {
+        refList.remove(this);
+        if (session.token.isValid()) {
+            Session newSession = null;
+            try {
+                newSession = session.token.getOpSession();
+                session.token.p11.C_DestroyObject(newSession.id(), keyID);
+            } catch (PKCS11Exception e) {
+                // ignore
+            } finally {
+                this.clear();
+                session.token.releaseSession(newSession);
+                session.removeObject();
+            }
+        }
+    }
+
+    public int compareTo(SessionKeyRef other) {
+        if (this.keyID == other.keyID) {
+            return 0;
+        } else {
+            return (this.keyID < other.keyID) ? -1 : 1;
+        }
+    }
 }
--- a/src/share/classes/sun/security/pkcs11/P11Mac.java	Wed Aug 04 00:07:49 2010 +0100
+++ b/src/share/classes/sun/security/pkcs11/P11Mac.java	Fri Aug 06 14:56:01 2010 -0700
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2007, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2010, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -263,16 +263,4 @@
             throw new ProviderException("update() failed", e);
         }
     }
-
-    protected void finalize() throws Throwable {
-        try {
-            if ((session != null) && token.isValid()) {
-                cancelOperation();
-                session = token.releaseSession(session);
-            }
-        } finally {
-            super.finalize();
-        }
-    }
-
 }
--- a/src/share/classes/sun/security/pkcs11/P11RSACipher.java	Wed Aug 04 00:07:49 2010 +0100
+++ b/src/share/classes/sun/security/pkcs11/P11RSACipher.java	Fri Aug 06 14:56:01 2010 -0700
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2007, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2010, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -98,7 +98,6 @@
         this.token = token;
         this.algorithm = "RSA";
         this.mechanism = mechanism;
-        session = token.getOpSession();
     }
 
     // modes do not make sense for RSA, but allow ECB
@@ -462,18 +461,6 @@
         int n = P11KeyFactory.convertKey(token, key, algorithm).keyLength();
         return n;
     }
-
-    protected void finalize() throws Throwable {
-        try {
-            if ((session != null) && token.isValid()) {
-                cancelOperation();
-                session = token.releaseSession(session);
-            }
-        } finally {
-            super.finalize();
-        }
-    }
-
 }
 
 final class ConstructKeys {
--- a/src/share/classes/sun/security/pkcs11/P11Signature.java	Wed Aug 04 00:07:49 2010 +0100
+++ b/src/share/classes/sun/security/pkcs11/P11Signature.java	Fri Aug 06 14:56:01 2010 -0700
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2006, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2010, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -234,7 +234,6 @@
         default:
             throw new ProviderException("Unknown mechanism: " + mechanism);
         }
-        session = token.getOpSession();
     }
 
     private void ensureInitialized() {
@@ -684,16 +683,4 @@
             throws InvalidParameterException {
         throw new UnsupportedOperationException("getParameter() not supported");
     }
-
-    protected void finalize() throws Throwable {
-        try {
-            if ((session != null) && token.isValid()) {
-                cancelOperation();
-                session = token.releaseSession(session);
-            }
-        } finally {
-            super.finalize();
-        }
-    }
-
 }
--- a/src/share/classes/sun/security/pkcs11/Session.java	Wed Aug 04 00:07:49 2010 +0100
+++ b/src/share/classes/sun/security/pkcs11/Session.java	Fri Aug 06 14:56:01 2010 -0700
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2005, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2010, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -25,6 +25,7 @@
 
 package sun.security.pkcs11;
 
+import java.lang.ref.*;
 import java.util.*;
 import java.util.concurrent.atomic.AtomicInteger;
 
@@ -59,11 +60,14 @@
     // this could lead to idle sessions being closed early, but that is harmless
     private long lastAccess;
 
+    private final SessionRef sessionRef;
+
     Session(Token token, long id) {
         this.token = token;
         this.id = id;
         createdObjects = new AtomicInteger();
         id();
+        sessionRef = new SessionRef(this, id, token);
     }
 
     public int compareTo(Session other) {
@@ -108,4 +112,76 @@
         return createdObjects.get() != 0;
     }
 
+    void close() {
+        if (hasObjects()) {
+            throw new ProviderException(
+                "Internal error: close session with active objects");
+        }
+        sessionRef.dispose();
+    }
 }
+
+/*
+ * NOTE: Use PhantomReference here and not WeakReference
+ * otherwise the sessions maybe closed before other objects
+ * which are still being finalized.
+ */
+final class SessionRef extends PhantomReference<Session>
+        implements Comparable<SessionRef> {
+
+    private static ReferenceQueue<Session> refQueue =
+        new ReferenceQueue<Session>();
+
+    private static Set<SessionRef> refList =
+        Collections.synchronizedSortedSet(new TreeSet<SessionRef>());
+
+    static ReferenceQueue<Session> referenceQueue() {
+        return refQueue;
+    }
+
+    static int totalCount() {
+        return refList.size();
+    }
+
+    private static void drainRefQueueBounded() {
+        while (true) {
+            SessionRef next = (SessionRef) refQueue.poll();
+            if (next == null) break;
+            next.dispose();
+        }
+    }
+
+    // handle to the native session
+    private long id;
+    private Token token;
+
+    SessionRef(Session session, long id, Token token) {
+        super(session, refQueue);
+        this.id = id;
+        this.token = token;
+        refList.add(this);
+        // TBD: run at some interval and not every time?
+        drainRefQueueBounded();
+    }
+
+    void dispose() {
+        refList.remove(this);
+        try {
+            token.p11.C_CloseSession(id);
+        } catch (PKCS11Exception e1) {
+            // ignore
+        } catch (ProviderException e2) {
+            // ignore
+        } finally {
+            this.clear();
+        }
+    }
+
+    public int compareTo(SessionRef other) {
+        if (this.id == other.id) {
+            return 0;
+        } else {
+            return (this.id < other.id) ? -1 : 1;
+        }
+    }
+}
--- a/src/share/classes/sun/security/pkcs11/SessionManager.java	Wed Aug 04 00:07:49 2010 +0100
+++ b/src/share/classes/sun/security/pkcs11/SessionManager.java	Fri Aug 06 14:56:01 2010 -0700
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2006, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2010, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -51,10 +51,12 @@
  * number of such sessions low. Note that we occasionally want to explicitly
  * close a session, see P11Signature.
  *
- * NOTE that all sessions obtained from this class MUST be returned using
- * either releaseSession() or closeSession() using a finally block or a
- * finalizer where appropriate. Otherwise, they will be "lost", i.e. there
- * will be a resource leak eventually leading to exhaustion.
+ * NOTE that sessions obtained from this class SHOULD be returned using
+ * either releaseSession() or closeSession() using a finally block when
+ * not needed anymore. Otherwise, they will be left for cleanup via the
+ * PhantomReference mechanism when GC kicks in, but it's best not to rely
+ * on that since GC may not run timely enough since the native PKCS11 library
+ * is also consuming memory.
  *
  * Note that sessions are automatically closed when they are not used for a
  * period of time, see Session.
@@ -74,9 +76,6 @@
     // maximum number of sessions to open with this token
     private final int maxSessions;
 
-    // total number of active sessions
-    private int activeSessions;
-
     // pool of available object sessions
     private final Pool objSessions;
 
@@ -116,6 +115,11 @@
         return (maxSessions <= DEFAULT_MAX_SESSIONS);
     }
 
+    // returns the total number of active sessions
+    int totalSessionCount() {
+        return SessionRef.totalCount();
+    }
+
     synchronized Session getObjSession() throws PKCS11Exception {
         Session session = objSessions.poll();
         if (session != null) {
@@ -136,7 +140,8 @@
         }
         // create a new session rather than re-using an obj session
         // that avoids potential expensive cancels() for Signatures & RSACipher
-        if (activeSessions < maxSessions) {
+        if (maxSessions == Integer.MAX_VALUE ||
+                totalSessionCount() < maxSessions) {
             session = openSession();
             return ensureValid(session);
         }
@@ -159,14 +164,10 @@
         if (debug != null) {
             String location = new Exception().getStackTrace()[2].toString();
             System.out.println("Killing session (" + location + ") active: "
-                + activeSessions);
+                + totalSessionCount());
         }
-        try {
-            closeSession(session);
-            return null;
-        } catch (PKCS11Exception e) {
-            throw new ProviderException(e);
-        }
+        closeSession(session);
+        return null;
     }
 
     synchronized Session releaseSession(Session session) {
@@ -187,7 +188,8 @@
             return;
         }
         if (debug != null) {
-            System.out.println("Demoting session, active: " + activeSessions);
+            System.out.println("Demoting session, active: " +
+                totalSessionCount());
         }
         boolean present = objSessions.remove(session);
         if (present == false) {
@@ -199,16 +201,17 @@
     }
 
     private Session openSession() throws PKCS11Exception {
-        if (activeSessions >= maxSessions) {
+        if ((maxSessions != Integer.MAX_VALUE) &&
+                (totalSessionCount() >= maxSessions)) {
             throw new ProviderException("No more sessions available");
         }
         long id = token.p11.C_OpenSession
                     (token.provider.slotID, openSessionFlags, null, null);
         Session session = new Session(token, id);
-        activeSessions++;
         if (debug != null) {
-            if (activeSessions > maxActiveSessions) {
-                maxActiveSessions = activeSessions;
+            int currTotal = totalSessionCount();
+            if (currTotal > maxActiveSessions) {
+                maxActiveSessions = currTotal;
                 if (maxActiveSessions % 10 == 0) {
                     System.out.println("Open sessions: " + maxActiveSessions);
                 }
@@ -217,13 +220,8 @@
         return session;
     }
 
-    private void closeSession(Session session) throws PKCS11Exception {
-        if (session.hasObjects()) {
-            throw new ProviderException
-                ("Internal error: close session with active objects");
-        }
-        token.p11.C_CloseSession(session.id());
-        activeSessions--;
+    private void closeSession(Session session) {
+        session.close();
     }
 
     private static final class Pool {
@@ -267,28 +265,20 @@
             }
             Collections.sort(pool);
             int i = 0;
-            PKCS11Exception exc = null;
             while (i < n - 1) { // always keep at least 1 session open
                 oldestSession = pool.get(i);
                 if (oldestSession.isLive(time)) {
                     break;
                 }
                 i++;
-                try {
-                    mgr.closeSession(oldestSession);
-                } catch (PKCS11Exception e) {
-                    exc = e;
-                }
+                mgr.closeSession(oldestSession);
             }
             if (debug != null) {
                 System.out.println("Closing " + i + " idle sessions, active: "
-                        + mgr.activeSessions);
+                        + mgr.totalSessionCount());
             }
             List<Session> subList = pool.subList(0, i);
             subList.clear();
-            if (exc != null) {
-                throw new ProviderException(exc);
-            }
         }
 
     }