From aec6b65fcbf819bc6cb83afd3728de3f7bdb2d91 Mon Sep 17 00:00:00 2001 From: Todd Lipcon Date: Tue, 3 Jul 2012 20:55:10 +0000 Subject: [PATCH] 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/branches/branch-2@1356937 13f79535-47bb-0310-9956-ffa450edef68 --- .../org/apache/hadoop/util/ServletUtil.java | 14 +++++ hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt | 2 + .../hdfs/server/namenode/GetImageServlet.java | 62 ++++++++++--------- .../hdfs/server/namenode/TransferFsImage.java | 16 ++--- 4 files changed, 58 insertions(+), 36 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ServletUtil.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ServletUtil.java index 993394d59bf..a401f3f65a4 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ServletUtil.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ServletUtil.java @@ -60,6 +60,20 @@ public class ServletUtil { 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 = "
\n" + "Hadoop, " diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt index cdfdec2328a..c26bda149f4 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt @@ -249,6 +249,8 @@ Release 2.0.1-alpha - UNRELEASED 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) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/GetImageServlet.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/GetImageServlet.java index 958cb14b9db..074fd237e54 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/GetImageServlet.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/GetImageServlet.java @@ -45,8 +45,10 @@ import org.apache.hadoop.hdfs.server.protocol.RemoteEditLog; 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; @@ -125,23 +127,14 @@ public class GetImageServlet extends HttpServlet { 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(); @@ -181,6 +174,28 @@ public class GetImageServlet extends HttpServlet { } 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) { @@ -192,7 +207,7 @@ public class GetImageServlet extends HttpServlet { } } - private static void setFileNameHeaders(HttpServletResponse response, + public static void setFileNameHeaders(HttpServletResponse response, File file) { response.setHeader(CONTENT_DISPOSITION, "attachment; filename=" + file.getName()); @@ -204,7 +219,7 @@ public class GetImageServlet extends HttpServlet { * @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); @@ -262,7 +277,7 @@ public class GetImageServlet extends HttpServlet { * 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())); @@ -335,7 +350,7 @@ public class GetImageServlet extends HttpServlet { 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; @@ -345,11 +360,11 @@ public class GetImageServlet extends HttpServlet { } } 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)) { @@ -405,16 +420,5 @@ public class GetImageServlet extends HttpServlet { 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); - } } } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/TransferFsImage.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/TransferFsImage.java index 492cf28fbe7..cc477084387 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/TransferFsImage.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/TransferFsImage.java @@ -25,6 +25,8 @@ import java.util.ArrayList; 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; @@ -145,16 +147,16 @@ public class TransferFsImage { * 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)) { @@ -178,14 +180,14 @@ public class TransferFsImage { 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(); } } }