Fix ProtocolError single-argument (subtype only) constructor

Correctly initialise mType from the ErrorType CONSTANT in the protocol message definition in this case, instead of leaving it uninitialised. Add test. Also test that these unexpected errors cause the connection to be closed as expected.
boxbackup · Dec 4, 2017 · a84c436 · a84c436
1 parent 02871a9
commit a84c436
Show file tree

Hide file tree

Showing 4 changed files with 74 additions and 14 deletions.
diff --git a/lib/server/makeprotocol.pl.in b/lib/server/makeprotocol.pl.in
@@ -118,11 +118,11 @@ while(<IN>)
 		my ($type,$name,$value) = split /\s+/,$l;
 		if($type eq 'CONSTANT')
 		{
-			push @{$cmd_constants{$current_cmd}},"$name = $value"
+			push @{$cmd_constants{$current_cmd}}, [$name, $value];
 		}
 		else
 		{
-			push @{$cmd_contents{$current_cmd}},$type,$name;
+			push @{$cmd_contents{$current_cmd}}, $type, $name;
 		}
 	}
 	else
@@ -317,8 +317,11 @@ __E
 	# constants
 	if(exists $cmd_constants{$cmd})
 	{
-		print H "\tenum\n\t{\n\t\t";
-		print H join(",\n\t\t",@{$cmd_constants{$cmd}});
+		print H "\tenum\n\t{\n";
+		foreach my $constant (@{$cmd_constants{$cmd}})
+		{
+			print H "\t\t", $constant->[0], " = ", $constant->[1], ",\n";
+		}
 		print H "\n\t};\n";
 	}
 
@@ -332,9 +335,19 @@ __E
 	{
 		$error_message = $cmd;
 		my ($mem_type,$mem_subtype) = split /,/,obj_get_type_params($cmd,'IsError');
-		my $error_type = $cmd_constants{"ErrorType"};
+		my %constants_hash;
+		my $error_type_value = "";
+
+		foreach my $constant (@{$cmd_constants{$cmd}})
+		{
+			if($constant->[0] eq "ErrorType")
+			{
+				$error_type_value = $constant->[0];
+			}
+		}
+
 		print H <<__E;
-	$cmd_class(int SubType) : m$mem_type($error_type), m$mem_subtype(SubType) { }
+	$cmd_class(int SubType) : m$mem_type($error_type_value), m$mem_subtype(SubType) { }
 	bool IsError(int &rTypeOut, int &rSubTypeOut) const;
 	std::string GetMessage() const { return GetMessage(m$mem_subtype); };
 	static std::string GetMessage(int subtype);
@@ -515,13 +528,13 @@ std::string $cmd_class\::GetMessage(int subtype)
 	switch(subtype)
 	{
 __E
-		foreach my $const (@{$cmd_constants{$cmd}})
+		foreach my $constant (@{$cmd_constants{$cmd}})
 		{
-			next unless $const =~ /^Err_(.*)/;
+			my $enum_name = $constant->[0];
+			next unless $enum_name =~ /^Err_(.*)/;
 			my $shortname = $1;
-			$const =~ s/ = .*//;
 			print CPP <<__E;
-		case $const: return "$shortname";
+		case $enum_name: return "$shortname";
 __E
 		}
 		print CPP <<__E;

diff --git a/test/basicserver/TestCommands.cpp b/test/basicserver/TestCommands.cpp
@@ -106,3 +106,16 @@ std::auto_ptr<TestProtocolMessage> TestProtocolString::DoCommand(TestProtocolRep
 	return std::auto_ptr<TestProtocolMessage>(new TestProtocolString(mTest));
 }
 
+std::auto_ptr<TestProtocolMessage> TestProtocolDeliberateError::DoCommand(
+	TestProtocolReplyable &rProtocol, TestContext &rContext) const
+{
+	return std::auto_ptr<TestProtocolMessage>(new TestProtocolError(
+		TestProtocolError::ErrorType,
+		TestProtocolError::Err_DeliberateError));
+}
+
+std::auto_ptr<TestProtocolMessage> TestProtocolUnexpectedError::DoCommand(
+	TestProtocolReplyable &rProtocol, TestContext &rContext) const
+{
+	THROW_EXCEPTION_MESSAGE(CommonException, Internal, "unexpected error");
+}
diff --git a/test/basicserver/TestProtocol.txt b/test/basicserver/TestProtocol.txt
@@ -9,6 +9,8 @@ BEGIN_OBJECTS
 Error		0	IsError(Type,SubType)	Reply
 	int32	Type
 	int32	SubType
+	CONSTANT	ErrorType		1000
+	CONSTANT	Err_DeliberateError	1
 
 Hello		1	Command(Hello)	Reply
 	int32	Number32
@@ -40,3 +42,6 @@ SendStream	8	Command(GetStream)	StreamWithCommand
 String		9	Command(String)	Reply
 	string	Test
 
+DeliberateError		10	Command(String)
+
+UnexpectedError		11	Command(String)
diff --git a/test/basicserver/testbasicserver.cpp b/test/basicserver/testbasicserver.cpp
@@ -524,6 +524,11 @@ int test(int argc, const char *argv[])
 		}
 	}
 
+	#ifndef WIN32
+	// Don't die quietly if the server dies while we're communicating with it
+	signal(SIGPIPE, SIG_IGN);
+	#endif
+
 	//printf("SKIPPING TESTS------------------------\n");
 	//goto protocolserver;
 
@@ -801,11 +806,35 @@ int test(int argc, const char *argv[])
 				TEST_THAT(reply->GetNumber8() == 22);
 				TEST_THAT(reply->GetText() == "Hello world!");
 			}
+
+			// Try to trigger a deliberate and an unexpected error (the latter
+			// by throwing an exception that isn't caught and handled by
+			// HandleException):
+			{
+				TEST_CHECK_THROWS(protocol.QueryDeliberateError(),
+					ConnectionException, Protocol_UnexpectedReply);
+				int type, subtype;
+				protocol.GetLastError(type, subtype);
+				TEST_EQUAL(TestProtocolError::ErrorType, type);
+				TEST_EQUAL(TestProtocolError::Err_DeliberateError, subtype);
+			}
+
+			{
+				TEST_CHECK_THROWS(protocol.QueryUnexpectedError(),
+					ConnectionException, Protocol_UnexpectedReply);
+				int type, subtype;
+				protocol.GetLastError(type, subtype);
+				TEST_EQUAL(TestProtocolError::ErrorType, type);
+				TEST_EQUAL(-1, subtype);
+			}
+
+			// The unexpected exception should kill the server child process that we
+			// connected to (except on Windows where the server does not fork a child),
+			// so we cannot communicate with it any more:
+			TEST_CHECK_THROWS(protocol.QueryQuit(),
+				ConnectionException, SocketWriteError);
 
-			// Quit query to finish
-			protocol.QueryQuit();
-
-			// Kill it
+			// Kill the main server process:
 			TEST_THAT(KillServer(pid));
 			::sleep(1);
 			TEST_THAT(!ServerIsAlive(pid));