Improve exception handling in S3Simulator, S3Client and HTTPServer.

Centralise error response generation in S3Simulator, driven by throwing and catching exceptions. Improve error messages returned by HTTPServer. Improve exception messages thrown by S3Client. Add a method to BoxException to quickly check what type of exception it is, and a macro to make this even shorter. Show the exception type as well as subtype in GetMessage().
boxbackup · Jul 23, 2017 · 83d68a4 · 83d68a4
1 parent 7e7ba9b
commit 83d68a4
Show file tree

Hide file tree

Showing 8 changed files with 50 additions and 35 deletions.
diff --git a/lib/common/BoxException.h b/lib/common/BoxException.h
@@ -29,11 +29,17 @@ class BoxException : public std::exception
 
 	virtual unsigned int GetType() const throw() = 0;
 	virtual unsigned int GetSubType() const throw() = 0;
+	bool IsType(unsigned int Type, unsigned int SubType)
+	{
+		return GetType() == Type && GetSubType() == SubType;
+	}
 	virtual const std::string& GetMessage() const = 0;
 
 private:
 };
 
+#define EXCEPTION_IS_TYPE(exception_obj, type, subtype) \
+	exception_obj.IsType(type::ExceptionType, type::subtype)
 
 #endif // BOXEXCEPTION__H
 
diff --git a/lib/common/CollectInBufferStream.h b/lib/common/CollectInBufferStream.h
@@ -42,6 +42,11 @@ class CollectInBufferStream : public IOStream
 	virtual pos_type BytesLeftToRead();
 	virtual void Write(const void *pBuffer, int NBytes,
 		int Timeout = IOStream::TimeOutInfinite);
+	virtual void Write(const std::string& rBuffer,
+		int Timeout = IOStream::TimeOutInfinite)
+	{
+		IOStream::Write(rBuffer, Timeout);
+	}
 	virtual pos_type GetPosition() const;
 	virtual void Seek(pos_type Offset, int SeekType);
 	virtual bool StreamDataLeft();

diff --git a/lib/common/makeexception.pl.in b/lib/common/makeexception.pl.in
@@ -160,12 +160,13 @@ for(my $e = 0; $e <= $#exception; $e++)
 {
 	if($exception[$e] ne '')
 	{
-		print CPP "\t\tcase ".$exception[$e].': return "'.$exception[$e].'";'."\n";
+		print CPP "\t\tcase ".$exception[$e].': return "'.$class.'Exception'.
+			'('.$exception[$e].')";'."\n";
 	}
 }
 
 print CPP <<__E;
-		default: return "Unknown";
+		default: return "${class}Exception(Unknown)";
 	}
 }
 __E

diff --git a/lib/httpserver/HTTPException.txt b/lib/httpserver/HTTPException.txt
@@ -15,3 +15,8 @@ BadResponse				11
 ResponseReadFailed			12
 NoStreamConfigured			13
 RequestFailedUnexpectedly		14	The request was expected to succeed, but it failed.
+ContentLengthAlreadySet			15	Tried to send a request without content, but the Content-Length header is already set.
+WrongContentLength			16	There was too much or not enough data in the request content stream.
+ParameterNotFound			17	An expected parameter was not found in the request.
+DuplicateParameter			18	A parameter was unexpectedly duplicated in the request.
+AuthenticationFailed			19	The request could not be authenticated.
diff --git a/lib/httpserver/HTTPServer.cpp b/lib/httpserver/HTTPServer.cpp
@@ -214,17 +214,16 @@ void HTTPServer::SendInternalErrorResponse(const std::string& rErrorMsg,
 {
 	#define ERROR_HTML_1 "<html><head><title>Internal Server Error</title></head>\n" \
 			"<h1>Internal Server Error</h1>\n" \
-			"<p>An error, type "
-	#define ERROR_HTML_2 " occured when processing the request.</p>" \
-			"<p>Please try again later.</p>" \
+			"<p>An error occurred while processing the request:</p>\n<pre>"
+	#define ERROR_HTML_2 "</pre>\n<p>Please try again later.</p>" \
 			"</body>\n</html>\n"
 
 	// Generate the error page
-	// rResponse.SetResponseCode(HTTPResponse::Code_InternalServerError);
+	rResponse.SetResponseCode(HTTPResponse::Code_InternalServerError);
 	rResponse.SetContentType("text/html");
-	rResponse.Write(ERROR_HTML_1, sizeof(ERROR_HTML_1) - 1);
-	rResponse.IOStream::Write(rErrorMsg.c_str());
-	rResponse.Write(ERROR_HTML_2, sizeof(ERROR_HTML_2) - 1);
+	rResponse.Write(ERROR_HTML_1);
+	rResponse.Write(rErrorMsg);
+	rResponse.Write(ERROR_HTML_2);
 }
 
 

diff --git a/lib/httpserver/S3Client.cpp b/lib/httpserver/S3Client.cpp
@@ -286,7 +286,8 @@ void S3Client::CheckResponse(const HTTPResponse& response, const std::string& me
 	if(response.GetResponseCode() != HTTPResponse::Code_OK)
 	{
 		THROW_EXCEPTION_MESSAGE(HTTPException, RequestFailedUnexpectedly,
-			message);
+			message << ": " <<
+			HTTPResponse::ResponseCodeToString(response.GetResponseCode()));
 	}
 }
 
diff --git a/lib/httpserver/S3Simulator.cpp b/lib/httpserver/S3Simulator.cpp
@@ -167,9 +167,8 @@ void S3Simulator::Handle(HTTPRequest &rRequest, HTTPResponse &rResponse)
 		if (!rRequest.GetHeader("authorization", &actualAuth) ||
 			actualAuth != expectedAuth)
 		{
-			rResponse.SetResponseCode(HTTPResponse::Code_Unauthorized);
-			SendInternalErrorResponse("Authentication Failed",
-				rResponse);
+			THROW_EXCEPTION_MESSAGE(HTTPException, AuthenticationFailed,
+				"Authentication code mismatch");
 		}
 		else if (rRequest.GetMethod() == HTTPRequest::Method_GET)
 		{
@@ -181,14 +180,27 @@ void S3Simulator::Handle(HTTPRequest &rRequest, HTTPResponse &rResponse)
 		}
 		else
 		{
-			rResponse.SetResponseCode(HTTPResponse::Code_MethodNotAllowed);
-			SendInternalErrorResponse("Unsupported Method",
-				rResponse);
+			THROW_EXCEPTION_MESSAGE(HTTPException, BadRequest,
+				"Unsupported Amazon S3 Method");
 		}
 	}
-	catch (CommonException &ce)
+	catch (BoxException &ce)
 	{
 		SendInternalErrorResponse(ce.what(), rResponse);
+
+		// Override the default status code 500 for a few specific exceptions.
+		if(EXCEPTION_IS_TYPE(ce, CommonException, OSFileOpenError))
+		{
+			rResponse.SetResponseCode(HTTPResponse::Code_NotFound);
+		}
+		else if(EXCEPTION_IS_TYPE(ce, CommonException, AccessDenied))
+		{
+			rResponse.SetResponseCode(HTTPResponse::Code_Forbidden);
+		}
+		else if(EXCEPTION_IS_TYPE(ce, HTTPException, AuthenticationFailed))
+		{
+			rResponse.SetResponseCode(HTTPResponse::Code_Unauthorized);
+		}
 	}
 	catch (std::exception &e)
 	{
@@ -231,22 +243,7 @@ void S3Simulator::HandleGet(HTTPRequest &rRequest, HTTPResponse &rResponse)
 	path += rRequest.GetRequestURI();
 	std::auto_ptr<FileStream> apFile;
 
-	try
-	{
-		apFile.reset(new FileStream(path));
-	}
-	catch (CommonException &ce)
-	{
-		if (ce.GetSubType() == CommonException::OSFileOpenError)
-		{
-			rResponse.SetResponseCode(HTTPResponse::Code_NotFound);
-		}
-		else if (ce.GetSubType() == CommonException::AccessDenied)
-		{
-			rResponse.SetResponseCode(HTTPResponse::Code_Forbidden);
-		}
-		throw;
-	}
+	apFile.reset(new FileStream(path));
 
 	// http://docs.amazonwebservices.com/AmazonS3/2006-03-01/UsingRESTOperations.html
 	apFile->CopyStreamTo(rResponse);

diff --git a/test/httpserver/testhttpserver.cpp b/test/httpserver/testhttpserver.cpp
@@ -304,8 +304,9 @@ int test(int argc, const char *argv[])
 		TEST_EQUAL("<html><head>"
 			"<title>Internal Server Error</title></head>\n"
 			"<h1>Internal Server Error</h1>\n"
-			"<p>An error, type Authentication Failed occured "
-			"when processing the request.</p>"
+			"<p>An error occurred while processing the request:</p>\n"
+			"<pre>HTTPException(AuthenticationFailed): "
+			"Authentication code mismatch</pre>\n"
 			"<p>Please try again later.</p></body>\n"
 			"</html>\n", response_data);
 	}