ZOOKEEPER-1246. Dead code in PrepRequestProcessor catch Exception block (camille)

git-svn-id: https://svn.apache.org/repos/asf/zookeeper/trunk@1196025 13f79535-47bb-0310-9956-ffa450edef68

CHANGES.txt

@@ -39,6 +39,8 @@ BUGFIXES:
   (Daniel Gómez Ferro via phunt)
   
   ZOOKEEPER-1264. FollowerResyncConcurrencyTest failing intermittently. (phunt via camille)
+  
+  ZOOKEEPER-1246. Dead code in PrepRequestProcessor catch Exception block. (camille)
 
 IMPROVEMENTS:
 

src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java

@@ -19,6 +19,7 @@
 package org.apache.zookeeper.server;
 
 import java.io.ByteArrayOutputStream;
+import java.io.IOException;
 import java.nio.ByteBuffer;
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -289,12 +290,18 @@ public class PrepRequestProcessor extends Thread implements RequestProcessor {
      * @param request
      * @param record
      */
-    protected void pRequest2Txn(int type, long zxid, Request request, Record record) throws KeeperException {
+    protected void pRequest2Txn(int type, long zxid, Request request,
+                                Record record, boolean deserialize)
+        throws KeeperException, IOException
+    {
         request.setHdr(new TxnHeader(request.sessionId, request.cxid, zxid, zks.getTime(), type));
 
         switch (type) {
             case OpCode.create:
                 CreateRequest createRequest = (CreateRequest)record;
+                if (deserialize) {
+                    ByteBufferInputStream.byteBuffer2Record(request.request, createRequest);
+                }
                 String path = createRequest.getPath();
                 int lastSlash = path.lastIndexOf('/');
                 if (lastSlash == -1 || path.indexOf('\0') != -1 || failCreate) {
@@ -348,6 +355,8 @@ public class PrepRequestProcessor extends Thread implements RequestProcessor {
                 break;
             case OpCode.delete:
                 DeleteRequest deleteRequest = (DeleteRequest)record;
+                if(deserialize)
+                    ByteBufferInputStream.byteBuffer2Record(request.request, deleteRequest);
                 path = deleteRequest.getPath();
                 lastSlash = path.lastIndexOf('/');
                 if (lastSlash == -1 || path.indexOf('\0') != -1
@@ -370,6 +379,8 @@ public class PrepRequestProcessor extends Thread implements RequestProcessor {
                 break;
             case OpCode.setData:
                 SetDataRequest setDataRequest = (SetDataRequest)record;
+                if(deserialize)
+                    ByteBufferInputStream.byteBuffer2Record(request.request, setDataRequest);
                 path = setDataRequest.getPath();
                 nodeRecord = getRecordForPath(path);
                 checkACL(zks, nodeRecord.acl, ZooDefs.Perms.WRITE, request.authInfo);
@@ -381,6 +392,8 @@ public class PrepRequestProcessor extends Thread implements RequestProcessor {
                 break;
             case OpCode.setACL:
                 SetACLRequest setAclRequest = (SetACLRequest)record;
+                if(deserialize)
+                    ByteBufferInputStream.byteBuffer2Record(request.request, setAclRequest);
                 path = setAclRequest.getPath();
                 listACL = removeDuplicates(setAclRequest.getAcl());
                 if (!fixupACL(request.authInfo, listACL)) {
@@ -427,6 +440,8 @@ public class PrepRequestProcessor extends Thread implements RequestProcessor {
                 break;
             case OpCode.check:
                 CheckVersionRequest checkVersionRequest = (CheckVersionRequest)record;
+                if(deserialize)
+                    ByteBufferInputStream.byteBuffer2Record(request.request, checkVersionRequest);
                 path = checkVersionRequest.getPath();
                 nodeRecord = getRecordForPath(path);
                 checkACL(zks, nodeRecord.acl, ZooDefs.Perms.READ, request.authInfo);
@@ -464,34 +479,34 @@ public class PrepRequestProcessor extends Thread implements RequestProcessor {
             switch (request.type) {
                 case OpCode.create:
                 CreateRequest createRequest = new CreateRequest();
-                ByteBufferInputStream.byteBuffer2Record(request.request, createRequest);
-                pRequest2Txn(request.type, zks.getNextZxid(), request, createRequest);
+                pRequest2Txn(request.type, zks.getNextZxid(), request, createRequest, true);
                 break;
             case OpCode.delete:
-                DeleteRequest deleteRequest = new DeleteRequest();
-                ByteBufferInputStream.byteBuffer2Record(request.request, deleteRequest);
-                pRequest2Txn(request.type, zks.getNextZxid(), request, deleteRequest);
+                DeleteRequest deleteRequest = new DeleteRequest();               
+                pRequest2Txn(request.type, zks.getNextZxid(), request, deleteRequest, true);
                 break;
             case OpCode.setData:
-                SetDataRequest setDataRequest = new SetDataRequest();
-                ByteBufferInputStream.byteBuffer2Record(request.request, setDataRequest);
-                pRequest2Txn(request.type, zks.getNextZxid(), request, setDataRequest);
+                SetDataRequest setDataRequest = new SetDataRequest();                
+                pRequest2Txn(request.type, zks.getNextZxid(), request, setDataRequest, true);
                 break;
             case OpCode.setACL:
-                SetACLRequest setAclRequest = new SetACLRequest();
-                ByteBufferInputStream.byteBuffer2Record(request.request, setAclRequest);
-                pRequest2Txn(request.type, zks.getNextZxid(), request, setAclRequest);
+                SetACLRequest setAclRequest = new SetACLRequest();                
+                pRequest2Txn(request.type, zks.getNextZxid(), request, setAclRequest, true);
                 break;
             case OpCode.check:
-                CheckVersionRequest checkRequest = new CheckVersionRequest();
-                ByteBufferInputStream.byteBuffer2Record(request.request, checkRequest);
-                pRequest2Txn(request.type, zks.getNextZxid(), request, checkRequest);
+                CheckVersionRequest checkRequest = new CheckVersionRequest();              
+                pRequest2Txn(request.type, zks.getNextZxid(), request, checkRequest, true);
                 break;
             case OpCode.multi:
                 MultiTransactionRecord multiRequest = new MultiTransactionRecord();
-                ByteBufferInputStream.byteBuffer2Record(request.request, multiRequest);
+                try {
+                    ByteBufferInputStream.byteBuffer2Record(request.request, multiRequest);
+                } catch(IOException e) {
+                    request.setHdr(new TxnHeader(request.sessionId, request.cxid, zks.getNextZxid(),
+                                                 zks.getTime(), OpCode.multi));
+                   throw e;
+                }
                 List<Txn> txns = new ArrayList<Txn>();
-
                 //Each op in a multi-op must have the same zxid!
                 long zxid = zks.getNextZxid();
                 KeeperException ke = null;
@@ -516,7 +531,7 @@ public class PrepRequestProcessor extends Thread implements RequestProcessor {
                     /* Prep the request and convert to a Txn */
                     else {
                         try {
-                            pRequest2Txn(op.getType(), zxid, request, subrequest);
+                            pRequest2Txn(op.getType(), zxid, request, subrequest, false);
                             type = request.getHdr().getType();
                             txn = request.getTxn();
                         } catch (KeeperException e) {
@@ -554,7 +569,7 @@ public class PrepRequestProcessor extends Thread implements RequestProcessor {
             //create/close session don't require request record
             case OpCode.createSession:
             case OpCode.closeSession:
-                pRequest2Txn(request.type, zks.getNextZxid(), request, null);
+                pRequest2Txn(request.type, zks.getNextZxid(), request, null, true);
                 break;
             }
         } catch (KeeperException e) {

src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java

@@ -0,0 +1,121 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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
+ *
+ *     http://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 "AS IS" 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 org.apache.zookeeper.server;
+
+import static org.junit.Assert.*;
+
+import java.io.File;
+import java.io.PrintWriter;
+import java.nio.ByteBuffer;
+
+import org.apache.zookeeper.PortAssignment;
+import org.apache.zookeeper.KeeperException.Code;
+import org.apache.zookeeper.KeeperException.SessionExpiredException;
+import org.apache.zookeeper.KeeperException.SessionMovedException;
+import org.apache.zookeeper.ZooDefs.OpCode;
+import org.apache.zookeeper.server.PrepRequestProcessor;
+import org.apache.zookeeper.server.Request;
+import org.apache.zookeeper.server.RequestProcessor;
+import org.apache.zookeeper.server.ServerCnxnFactory;
+import org.apache.zookeeper.server.SyncRequestProcessor;
+import org.apache.zookeeper.server.ZooKeeperServer;
+import org.apache.zookeeper.test.ClientBase;
+import org.apache.zookeeper.txn.ErrorTxn;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class PrepRequestProcessorTest extends ClientBase {
+    private static String HOSTPORT = "127.0.0.1:" + PortAssignment.unique();
+    private static final int CONNECTION_TIMEOUT = 3000;
+
+    @Test
+    public void testPRequest() throws Exception {
+        File tmpDir = ClientBase.createTmpDir();
+        ClientBase.setupTestEnv();
+        ZooKeeperServer zks = new ZooKeeperServer(tmpDir, tmpDir, 3000);
+        SyncRequestProcessor.setSnapCount(100);
+        final int PORT = Integer.parseInt(HOSTPORT.split(":")[1]);
+        ServerCnxnFactory f = ServerCnxnFactory.createFactory(PORT, -1);
+        f.startup(zks);
+        Assert.assertTrue("waiting for server being up ",
+                ClientBase.waitForServerUp(HOSTPORT,CONNECTION_TIMEOUT));
+        zks.sessionTracker = new MySessionTracker(); 
+        PrepRequestProcessor processor = new PrepRequestProcessor(zks, new MyRequestProcessor());
+        Request foo = new Request(null, 1l, 1, OpCode.create, ByteBuffer.allocate(3), null);
+        processor.pRequest(foo);
+    }
+ 
+
+    private class MyRequestProcessor implements RequestProcessor {
+        @Override
+        public void processRequest(Request request) {
+            Assert.assertEquals("Request should have marshalling error", new ErrorTxn(Code.MARSHALLINGERROR.intValue()),  request.getTxn());
+        }
+        @Override
+        public void shutdown() {
+            // TODO Auto-generated method stub
+            
+        }
+    }
+    
+    private class MySessionTracker implements SessionTracker {
+        @Override
+        public void addSession(long id, int to) {
+            // TODO Auto-generated method stub
+            
+        }
+        @Override
+        public void checkSession(long sessionId, Object owner)
+                throws SessionExpiredException, SessionMovedException {
+            // TODO Auto-generated method stub
+            
+        }
+        @Override
+        public long createSession(int sessionTimeout) {
+            // TODO Auto-generated method stub
+            return 0;
+        }
+        @Override
+        public void dumpSessions(PrintWriter pwriter) {
+            // TODO Auto-generated method stub
+            
+        }
+         @Override
+        public void removeSession(long sessionId) {
+            // TODO Auto-generated method stub
+            
+        }
+        @Override
+        public void setOwner(long id, Object owner)
+                throws SessionExpiredException {
+            // TODO Auto-generated method stub
+            
+        }
+        @Override
+        public void shutdown() {
+            // TODO Auto-generated method stub
+            
+        }
+        @Override
+        public boolean touchSession(long sessionId, int sessionTimeout) {
+            // TODO Auto-generated method stub
+            return false;
+        }
+    }
+}