Delete Sessions which cause deserialization errors

Marcus Thiesen · Marcus Thiesen · commit 83d928931e4c · 2014-01-16T13:25:13.000+01:00
Trying to deserialize sessions that are for some reason not
deserializable leads to continous retries at the moment.

As the data is apparently not loadable, it might be a better idea
to remove the data from memcached alltogether in order to avoid
expensive retries.
diff --git a/core/src/main/java/de/javakaffee/web/msm/MemcachedSessionService.java b/core/src/main/java/de/javakaffee/web/msm/MemcachedSessionService.java
@@ -1100,25 +1100,32 @@ private MemcachedBackupSession loadFromMemcached( final String sessionId ) {
                 return result;
             }
             else {
-                if ( lockStatus == LockStatus.LOCKED ) {
-                    _lockingStrategy.releaseLock( sessionId );
-                }
+                releaseIfLocked( sessionId, lockStatus );
                 _invalidSessionsCache.put( sessionId, Boolean.TRUE );
                 if ( _log.isDebugEnabled() ) {
                     _log.debug( "Session " + sessionId + " not found in memcached." );
                 }
                 return null;
             }
-
+        } catch ( final TranscoderDeserializationException e ) {
+            _log.warn( "Could not deserialize session with id " + sessionId + " from memcached, session will be purged from storage.", e );
+            releaseIfLocked( sessionId, lockStatus );
+            _memcached.delete( sessionId );
+            _invalidSessionsCache.put( sessionId, Boolean.TRUE );
         } catch ( final Exception e ) {
             _log.warn( "Could not load session with id " + sessionId + " from memcached.", e );
-            if ( lockStatus == LockStatus.LOCKED ) {
-                _lockingStrategy.releaseLock( sessionId );
-            }
+            releaseIfLocked( sessionId, lockStatus );
+        } finally {
         }
         return null;
     }
 
+    protected void releaseIfLocked( final String sessionId, LockStatus lockStatus ) {
+        if ( lockStatus == LockStatus.LOCKED ) {
+            _lockingStrategy.releaseLock( sessionId );
+        }
+    }
+
     /**
      * Set the memcached nodes space or comma separated.
      * <p>
diff --git a/core/src/main/java/de/javakaffee/web/msm/TranscoderDeserializationException.java b/core/src/main/java/de/javakaffee/web/msm/TranscoderDeserializationException.java
@@ -0,0 +1,38 @@
+/*
+ * Copyright 2014 Marcus Thiesen
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an &quot;AS IS&quot; BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package de.javakaffee.web.msm;
+
+public class TranscoderDeserializationException extends RuntimeException {
+
+    private static final long serialVersionUID = 1L;
+
+    public TranscoderDeserializationException() {
+    }
+
+    public TranscoderDeserializationException( String message ) {
+        super( message );
+    }
+
+    public TranscoderDeserializationException( Throwable cause ) {
+        super( cause );
+    }
+
+    public TranscoderDeserializationException( String message, Throwable cause ) {
+        super( message, cause );
+    }
+
+}
diff --git a/core/src/test/java/de/javakaffee/web/msm/TranscoderServiceTest.java b/core/src/test/java/de/javakaffee/web/msm/TranscoderServiceTest.java
@@ -187,6 +187,7 @@ public void testSerializeSessionWithAttributes() {
 
         assertSessionFields( session, deserialized );
         Assert.assertEquals( value, deserialized.getAttribute( "foo" ) );
+
     }
 
     private void assertSessionFields( final MemcachedBackupSession session, final MemcachedBackupSession deserialized ) {
diff --git a/kryo-serializer/src/main/java/de/javakaffee/web/msm/serializer/kryo/KryoTranscoder.java b/kryo-serializer/src/main/java/de/javakaffee/web/msm/serializer/kryo/KryoTranscoder.java
@@ -43,6 +43,7 @@
 
 import com.esotericsoftware.kryo.Kryo;
 import com.esotericsoftware.kryo.ObjectBuffer;
+import com.esotericsoftware.kryo.SerializationException;
 import com.esotericsoftware.kryo.Serializer;
 import com.esotericsoftware.kryo.serialize.BigDecimalSerializer;
 import com.esotericsoftware.kryo.serialize.BigIntegerSerializer;
@@ -72,6 +73,7 @@
 import de.javakaffee.kryoserializers.UnmodifiableCollectionsSerializer;
 import de.javakaffee.web.msm.MemcachedBackupSession;
 import de.javakaffee.web.msm.SessionAttributesTranscoder;
+import de.javakaffee.web.msm.TranscoderDeserializationException;
 
 /**
  * A {@link SessionAttributesTranscoder} that uses {@link Kryo} for serialization.
@@ -261,7 +263,11 @@ private Serializer loadCopyCollectionSerializer( final Class<?> clazz, final Kry
     @SuppressWarnings( "unchecked" )
     @Override
     public Map<String, Object> deserializeAttributes( final byte[] data ) {
-        return new ObjectBuffer( _kryo ).readObject( data, ConcurrentHashMap.class );
+        try {
+            return new ObjectBuffer( _kryo ).readObject( data, ConcurrentHashMap.class );
+        } catch ( final SerializationException e ) {
+            throw new TranscoderDeserializationException( e );
+        }
     }
 
     /**
diff --git a/kryo-serializer/src/main/java/de/javakaffee/web/msm/serializer/kryo/KryoTranscoderFactory.java b/kryo-serializer/src/main/java/de/javakaffee/web/msm/serializer/kryo/KryoTranscoderFactory.java
@@ -16,7 +16,6 @@
  */
 package de.javakaffee.web.msm.serializer.kryo;
 
-import org.apache.catalina.Manager;
 import org.apache.juli.logging.Log;
 import org.apache.juli.logging.LogFactory;
 
@@ -44,7 +43,11 @@ public class KryoTranscoderFactory implements TranscoderFactory {
      * {@inheritDoc}
      */
     public SessionAttributesTranscoder createTranscoder( final SessionManager manager ) {
-        return getTranscoder( manager );
+        return getTranscoder( manager.getContainer().getLoader().getClassLoader() );
+    }
+
+    protected SessionAttributesTranscoder createTranscoder( final ClassLoader loader ) {
+        return getTranscoder( loader );
     }
 
     /**
@@ -54,11 +57,11 @@ public SessionAttributesTranscoder createTranscoder( final SessionManager manage
      * @param manager the manager that will be passed to the transcoder.
      * @return for all invocations the same instance of {@link JavolutionTranscoder}.
      */
-    private KryoTranscoder getTranscoder( final Manager manager ) {
+    private KryoTranscoder getTranscoder( final ClassLoader classLoader ) {
         if ( _transcoder == null ) {
             final int initialBufferSize = getSysPropValue( PROP_INIT_BUFFER_SIZE, KryoTranscoder.DEFAULT_INITIAL_BUFFER_SIZE );
             final int maxBufferSize = getSysPropValue( PROP_ENV_MAX_BUFFER_SIZE, KryoTranscoder.DEFAULT_MAX_BUFFER_SIZE );
-            _transcoder = new KryoTranscoder( manager.getContainer().getLoader().getClassLoader(),
+            _transcoder = new KryoTranscoder( classLoader,
                     _customConverterClassNames, _copyCollectionsForSerialization, initialBufferSize, maxBufferSize );
         }
         return _transcoder;
diff --git a/kryo-serializer/src/test/java/de/javakaffee/web/msm/serializer/kryo/ClassGenerationUtil.java b/kryo-serializer/src/test/java/de/javakaffee/web/msm/serializer/kryo/ClassGenerationUtil.java
@@ -0,0 +1,69 @@
+/*
+ * Copyright 2014 Marcus Thiesen
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an &quot;AS IS&quot; BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package de.javakaffee.web.msm.serializer.kryo;
+
+import javassist.ClassPool;
+import javassist.CtClass;
+import javassist.CtField;
+
+/**
+ * @author Marcus Thiesen (marcus.thiesen@freiheit.com) (initial creation)
+ */
+public class ClassGenerationUtil {
+
+    private static byte[] makeClass( final String name, final String... fields ) {
+        final ClassPool pool = ClassPool.getDefault();
+
+        final CtClass newClass = pool.makeClass( name );
+
+        try {
+            for ( final String field : fields ) {
+                newClass.addField( new CtField( pool.get( "java.lang.String"), field, newClass ) );
+            }
+
+            return newClass.toBytecode();
+        } catch ( Exception e ) {
+            throw new RuntimeException( e );
+        }
+    }
+
+    public static class ByteClassLoader extends ClassLoader {
+
+        private final String _className;
+        private final byte[] _classBytes;
+
+        public ByteClassLoader( final ClassLoader parent, final String className, byte[] classBytes ) {
+            super( parent);
+
+            _className = className;
+            _classBytes = classBytes;
+        }
+
+        @Override
+        protected Class<?> findClass(final String name) throws ClassNotFoundException {
+            if ( _className.equals( name ) ) {
+                return defineClass( name, _classBytes, 0, _classBytes.length ); 
+            }
+            return super.findClass(name);
+        }
+    }
+
+    public static ClassLoader makeClassLoaderForCustomClass( final ClassLoader parent, final String className, final String... fields ) {
+        return new ByteClassLoader( parent, className, makeClass( className, fields ) );
+    }
+
+}
diff --git a/kryo-serializer/src/test/java/de/javakaffee/web/msm/serializer/kryo/SerializationChangeTests.java b/kryo-serializer/src/test/java/de/javakaffee/web/msm/serializer/kryo/SerializationChangeTests.java
@@ -0,0 +1,80 @@
+/*
+ * Copyright 2014 Marcus Thiesen
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an &quot;AS IS&quot; BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+package de.javakaffee.web.msm.serializer.kryo;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertNotNull;
+
+import de.javakaffee.web.msm.MemcachedBackupSession;
+import de.javakaffee.web.msm.SessionAttributesTranscoder;
+import de.javakaffee.web.msm.TranscoderDeserializationException;
+import static de.javakaffee.web.msm.integration.TestUtils.assertDeepEquals;
+
+/**
+ * A test to verify that the correct exception is thrown when a class 
+ * show incompatible changes.
+ * 
+ * @author Marcus Thiesen (marcus.thiesen@freiheit.com) (initial creation)
+ */
+public class SerializationChangeTests {
+
+    private static final String TEST_TYPE_CLASS_NAME = "test.Type";
+
+    @Test( expectedExceptions = TranscoderDeserializationException.class )
+    public void testDeserializationError() {
+        // Create a class with one simple field:
+        final ClassLoader loaderForCustomClassInVersion1 = ClassGenerationUtil.makeClassLoaderForCustomClass( this.getClass().getClassLoader(), TEST_TYPE_CLASS_NAME, "field1" );
+        final Object value = makeValueInstance( loaderForCustomClassInVersion1 );
+
+        final SessionAttributesTranscoder transcoder = new KryoTranscoderFactory().createTranscoder( loaderForCustomClassInVersion1 );
+
+        // serialize one instance
+        final MemcachedBackupSession memcachedBackupSession = new MemcachedBackupSession();
+        final Map<String, Object> attributes = new HashMap<String, Object>();
+        attributes.put( "test", value );
+
+        byte[] data = transcoder.serializeAttributes( memcachedBackupSession, attributes );
+        final Map<String, Object> deserializeAttributes = transcoder.deserializeAttributes( data );
+
+        final Object actual = deserializeAttributes.get( "test" );
+        assertNotNull(actual);
+        assertDeepEquals(actual, value);
+
+        // create same class with second field
+        final ClassLoader loaderForCustomClassInVersion2 = ClassGenerationUtil.makeClassLoaderForCustomClass( this.getClass().getClassLoader(), TEST_TYPE_CLASS_NAME, "field1", "field2" );
+        final SessionAttributesTranscoder secondTranscoder = new KryoTranscoderFactory().createTranscoder( loaderForCustomClassInVersion2 );
+
+        // this should lead to an exception
+        secondTranscoder.deserializeAttributes( data );
+    }
+
+    private static Object makeValueInstance( ClassLoader loaderForCustomClass ) {
+        try {
+            Class<?> forName = Class.forName( TEST_TYPE_CLASS_NAME, true, loaderForCustomClass );
+
+            return forName.newInstance();
+
+        } catch ( Exception e ) {
+            throw new RuntimeException( e );
+        }
+    }
+
+}

Original file line number	Diff line number	Diff line change
`@@ -187,6 +187,7 @@ public void testSerializeSessionWithAttributes() {`
`187`	`187`
`188`	`188`	`assertSessionFields( session, deserialized );`
`189`	`189`	`Assert.assertEquals( value, deserialized.getAttribute( "foo" ) );`
	`190`	`+`
`190`	`191`	`}`
`191`	`192`
`192`	`193`	`private void assertSessionFields( final MemcachedBackupSession session, final MemcachedBackupSession deserialized ) {`