HDFS-3574. Fix small race and do some cleanup in GetImageServlet. Contributed by Todd Lipcon.

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

Author: toddlipcon

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ServletUtil.java

@@ -60,6 +60,20 @@ public static String getParameter(ServletRequest request, String name) {
     s = s.trim();
     return s.length() == 0? null: s;
   }
+  
+  /**
+   * @return a long value as passed in the given parameter, throwing
+   * an exception if it is not present or if it is not a valid number.
+   */
+  public static long parseLongParam(ServletRequest request, String param)
+      throws IOException {
+    String paramStr = request.getParameter(param);
+    if (paramStr == null) {
+      throw new IOException("Invalid request has no " + param + " parameter");
+    }
+    
+    return Long.valueOf(paramStr);
+  }
 
   public static final String HTML_TAIL = "<hr />\n"
     + "<a href='http://hadoop.apache.org/core'>Hadoop</a>, " 

hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

@@ -397,6 +397,8 @@ Branch-2 ( Unreleased changes )
 
     HDFS-3575. HttpFS does not log Exception Stacktraces (brocknoland via tucu)
 
+    HDFS-3574. Fix small race and do some cleanup in GetImageServlet (todd)
+
   BREAKDOWN OF HDFS-3042 SUBTASKS
 
     HDFS-2185. HDFS portion of ZK-based FailoverController (todd)

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/GetImageServlet.java

@@ -46,8 +46,10 @@
 import org.apache.hadoop.hdfs.util.DataTransferThrottler;
 import org.apache.hadoop.hdfs.util.MD5FileUtils;
 import org.apache.hadoop.http.HttpServer;
+import org.apache.hadoop.io.IOUtils;
 import org.apache.hadoop.io.MD5Hash;
 import org.apache.hadoop.security.UserGroupInformation;
+import org.apache.hadoop.util.ServletUtil;
 import org.apache.hadoop.util.StringUtils;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -126,23 +128,14 @@ public Void run() throws Exception {
               throw new IOException(errorMessage);
             }
             CheckpointFaultInjector.getInstance().beforeGetImageSetsHeaders();
-            setFileNameHeaders(response, imageFile);
-            setVerificationHeaders(response, imageFile);
-            // send fsImage
-            TransferFsImage.getFileServer(response.getOutputStream(), imageFile,
-                getThrottler(conf)); 
+            serveFile(imageFile);
           } else if (parsedParams.isGetEdit()) {
             long startTxId = parsedParams.getStartTxId();
             long endTxId = parsedParams.getEndTxId();
 
             File editFile = nnImage.getStorage()
                 .findFinalizedEditsFile(startTxId, endTxId);
-            setVerificationHeaders(response, editFile);
-            
-            setFileNameHeaders(response, editFile);
-            // send edits
-            TransferFsImage.getFileServer(response.getOutputStream(), editFile,
-                getThrottler(conf));
+            serveFile(editFile);
           } else if (parsedParams.isPutImage()) {
             final long txid = parsedParams.getTxId();
 
@@ -182,6 +175,28 @@ public Void run() throws Exception {
           }
           return null;
         }
+
+        private void serveFile(File file) throws IOException {
+          FileInputStream fis = new FileInputStream(file);
+          try {
+            setVerificationHeaders(response, file);
+            setFileNameHeaders(response, file);
+            if (!file.exists()) {
+              // Potential race where the file was deleted while we were in the
+              // process of setting headers!
+              throw new FileNotFoundException(file.toString());
+              // It's possible the file could be deleted after this point, but
+              // we've already opened the 'fis' stream.
+              // It's also possible length could change, but this would be
+              // detected by the client side as an inaccurate length header.
+            }
+            // send file
+            TransferFsImage.getFileServer(response, file, fis,
+                getThrottler(conf));
+          } finally {
+            IOUtils.closeStream(fis);
+          }
+        }
       });
 
     } catch (Throwable t) {
@@ -193,7 +208,7 @@ public Void run() throws Exception {
     }
   }
 
-  private static void setFileNameHeaders(HttpServletResponse response,
+  public static void setFileNameHeaders(HttpServletResponse response,
       File file) {
     response.setHeader(CONTENT_DISPOSITION, "attachment; filename=" +
         file.getName());
@@ -205,7 +220,7 @@ private static void setFileNameHeaders(HttpServletResponse response,
    * @param conf configuration
    * @return a data transfer throttler
    */
-  private final DataTransferThrottler getThrottler(Configuration conf) {
+  public final static DataTransferThrottler getThrottler(Configuration conf) {
     long transferBandwidth = 
       conf.getLong(DFSConfigKeys.DFS_IMAGE_TRANSFER_RATE_KEY,
                    DFSConfigKeys.DFS_IMAGE_TRANSFER_RATE_DEFAULT);
@@ -263,7 +278,7 @@ static boolean isValidRequestor(ServletContext context, String remoteUser,
    * Set headers for content length, and, if available, md5.
    * @throws IOException 
    */
-  private void setVerificationHeaders(HttpServletResponse response, File file)
+  public static void setVerificationHeaders(HttpServletResponse response, File file)
   throws IOException {
     response.setHeader(TransferFsImage.CONTENT_LENGTH,
         String.valueOf(file.length()));
@@ -336,7 +351,7 @@ public GetImageParams(HttpServletRequest request,
         if (key.equals("getimage")) { 
           isGetImage = true;
           try {
-            txId = parseLongParam(request, TXID_PARAM);
+            txId = ServletUtil.parseLongParam(request, TXID_PARAM);
           } catch (NumberFormatException nfe) {
             if (request.getParameter(TXID_PARAM).equals(LATEST_FSIMAGE_VALUE)) {
               fetchLatest = true;
@@ -346,11 +361,11 @@ public GetImageParams(HttpServletRequest request,
           }
         } else if (key.equals("getedit")) { 
           isGetEdit = true;
-          startTxId = parseLongParam(request, START_TXID_PARAM);
-          endTxId = parseLongParam(request, END_TXID_PARAM);
+          startTxId = ServletUtil.parseLongParam(request, START_TXID_PARAM);
+          endTxId = ServletUtil.parseLongParam(request, END_TXID_PARAM);
         } else if (key.equals("putimage")) { 
           isPutImage = true;
-          txId = parseLongParam(request, TXID_PARAM);
+          txId = ServletUtil.parseLongParam(request, TXID_PARAM);
         } else if (key.equals("port")) { 
           remoteport = new Integer(val[0]).intValue();
         } else if (key.equals(STORAGEINFO_PARAM)) {
@@ -406,16 +421,5 @@ boolean shouldFetchLatest() {
       return fetchLatest;
     }
 
-    private static long parseLongParam(HttpServletRequest request, String param)
-        throws IOException {
-      // Parse the 'txid' parameter which indicates which image is to be
-      // fetched.
-      String paramStr = request.getParameter(param);
-      if (paramStr == null) {
-        throw new IOException("Invalid request has no " + param + " parameter");
-      }
-      
-      return Long.valueOf(paramStr);
-    }
   }
 }

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/TransferFsImage.java

@@ -25,6 +25,8 @@
 import java.util.List;
 import java.lang.Math;
 
+import javax.servlet.ServletOutputStream;
+import javax.servlet.ServletResponse;
 import javax.servlet.http.HttpServletResponse;
 
 import org.apache.commons.logging.Log;
@@ -147,16 +149,16 @@ public static void uploadImageFromStorage(String fsName,
    * A server-side method to respond to a getfile http request
    * Copies the contents of the local file into the output stream.
    */
-  static void getFileServer(OutputStream outstream, File localfile,
+  public static void getFileServer(ServletResponse response, File localfile,
+      FileInputStream infile,
       DataTransferThrottler throttler) 
     throws IOException {
     byte buf[] = new byte[HdfsConstants.IO_FILE_BUFFER_SIZE];
-    FileInputStream infile = null;
+    ServletOutputStream out = null;
     try {
-      infile = new FileInputStream(localfile);
       CheckpointFaultInjector.getInstance()
           .aboutToSendFile(localfile);
-      
+      out = response.getOutputStream();
 
       if (CheckpointFaultInjector.getInstance().
             shouldSendShortFile(localfile)) {
@@ -180,14 +182,14 @@ static void getFileServer(OutputStream outstream, File localfile,
           buf[0]++;
         }
 
-        outstream.write(buf, 0, num);
+        out.write(buf, 0, num);
         if (throttler != null) {
           throttler.throttle(num);
         }
       }
     } finally {
-      if (infile != null) {
-        infile.close();
+      if (out != null) {
+        out.close();
       }
     }
   }