Commit 45f233c1 authored by Adam Simpkins's avatar Adam Simpkins Committed by Facebook GitHub Bot

use exceptionStr() when formating some errors in the logging code

Summary:
If an error is thrown when formating arguments for a log message, use
`folly::exceptionStr()` to format the exception instead of just `ex.what()`.

I changed all of the tests that verify the results of these strings to use a
regular expression, as `folly::exceptionStr()` produces different results in
different platforms, and does not always demangle type names.

I also included a minor change to increase a timeout in
AsyncFileWriterTest.cpp, as I saw this timeout fail spuriously once in my
testing.

Reviewed By: yfeldblum

Differential Revision: D21635680

fbshipit-source-id: 749ef48deb59af1f2faef0158e0291838e09fc38
parent 2d2c15fc
...@@ -19,6 +19,7 @@ ...@@ -19,6 +19,7 @@
#include <fmt/core.h> #include <fmt/core.h>
#include <folly/CPortability.h> #include <folly/CPortability.h>
#include <folly/Conv.h> #include <folly/Conv.h>
#include <folly/ExceptionString.h>
#include <folly/Portability.h> #include <folly/Portability.h>
#include <folly/lang/Exception.h> #include <folly/lang/Exception.h>
#include <folly/logging/LogCategory.h> #include <folly/logging/LogCategory.h>
...@@ -333,7 +334,7 @@ class LogStreamProcessor { ...@@ -333,7 +334,7 @@ class LogStreamProcessor {
// Just log an error message letting indicating that something went // Just log an error message letting indicating that something went
// wrong formatting the log message. // wrong formatting the log message.
return folly::to<std::string>( return folly::to<std::string>(
"error constructing log message: ", ex.what()); "error constructing log message: ", exceptionStr(ex));
}); });
} }
...@@ -356,7 +357,7 @@ class LogStreamProcessor { ...@@ -356,7 +357,7 @@ class LogStreamProcessor {
failed = true; failed = true;
std::string result; std::string result;
result.append("error formatting log message: "); result.append("error formatting log message: ");
result.append(ex.what()); result.append(exceptionStr(ex).c_str());
result.append("; format string: \""); result.append("; format string: \"");
result.append(fmt.data(), fmt.size()); result.append(fmt.data(), fmt.size());
result.append("\", arguments: "); result.append("\", arguments: ");
......
...@@ -258,7 +258,7 @@ TEST(AsyncFileWriter, flush) { ...@@ -258,7 +258,7 @@ TEST(AsyncFileWriter, flush) {
EXPECT_EQ(bytesRead, paddingSize); EXPECT_EQ(bytesRead, paddingSize);
// Make sure flush completes successfully now // Make sure flush completes successfully now
std::move(future).get(10ms); std::move(future).get(50ms);
} }
// A large-ish message suffix, just to consume space and help fill up // A large-ish message suffix, just to consume space and help fill up
......
...@@ -111,12 +111,12 @@ TEST_F(LoggerTest, formatError) { ...@@ -111,12 +111,12 @@ TEST_F(LoggerTest, formatError) {
ASSERT_EQ(1, messages.size()); ASSERT_EQ(1, messages.size());
// Use a regex match here, since the type IDs are reported slightly // Use a regex match here, since the type IDs are reported slightly
// differently on different platforms. // differently on different platforms.
EXPECT_EQ( EXPECT_THAT(
R"(error formatting log message: )" messages[0].first.getMessage(),
R"(invalid type specifier; )" MatchesRegex(R"(error formatting log message: )"
R"(format string: "param1: {:06d}, param2: {:6.3f}", )" R"(.*invalid type specifier; )"
R"(arguments: 1234, hello world!)", R"(format string: "param1: \{:06d\}, param2: \{:6.3f\}", )"
messages[0].first.getMessage()); R"(arguments: 1234, hello world!)"));
EXPECT_EQ("LoggerTest.cpp", pathBasename(messages[0].first.getFileName())); EXPECT_EQ("LoggerTest.cpp", pathBasename(messages[0].first.getFileName()));
EXPECT_EQ(LogLevel::WARN, messages[0].first.getLevel()); EXPECT_EQ(LogLevel::WARN, messages[0].first.getLevel());
EXPECT_FALSE(messages[0].first.containsNewlines()); EXPECT_FALSE(messages[0].first.containsNewlines());
...@@ -185,10 +185,10 @@ TEST_F(LoggerTest, toStringError) { ...@@ -185,10 +185,10 @@ TEST_F(LoggerTest, toStringError) {
auto& messages = handler_->getMessages(); auto& messages = handler_->getMessages();
ASSERT_EQ(1, messages.size()); ASSERT_EQ(1, messages.size());
EXPECT_EQ( EXPECT_THAT(
"error constructing log message: " messages[0].first.getMessage(),
"error converting ToStringFailure object to a string", MatchesRegex("error constructing log message: .*"
messages[0].first.getMessage()); "error converting ToStringFailure object to a string"));
EXPECT_EQ("LoggerTest.cpp", pathBasename(messages[0].first.getFileName())); EXPECT_EQ("LoggerTest.cpp", pathBasename(messages[0].first.getFileName()));
EXPECT_EQ(expectedLine, messages[0].first.getLineNumber()); EXPECT_EQ(expectedLine, messages[0].first.getLineNumber());
EXPECT_EQ(LogLevel::DBG1, messages[0].first.getLevel()); EXPECT_EQ(LogLevel::DBG1, messages[0].first.getLevel());
...@@ -207,7 +207,7 @@ TEST_F(LoggerTest, formatFallbackError) { ...@@ -207,7 +207,7 @@ TEST_F(LoggerTest, formatFallbackError) {
EXPECT_THAT( EXPECT_THAT(
messages[0].first.getMessage(), messages[0].first.getMessage(),
MatchesRegex( MatchesRegex(
R"(error formatting log message: argument index out of range; )" R"(error formatting log message: .*argument index out of range; )"
R"(format string: "param1: \{\}, param2: \{\}, \{\}", )" R"(format string: "param1: \{\}, param2: \{\}, \{\}", )"
R"(arguments: 1234, )" R"(arguments: 1234, )"
R"(\[(.*ToStringFailure.*|object) of size (.*):.*\])")); R"(\[(.*ToStringFailure.*|object) of size (.*):.*\])"));
...@@ -225,7 +225,7 @@ TEST_F(LoggerTest, formatFallbackUnsupported) { ...@@ -225,7 +225,7 @@ TEST_F(LoggerTest, formatFallbackUnsupported) {
std::string objectHex = kIsLittleEndian ? "ef cd 34 12" : "12 34 cd ef"; std::string objectHex = kIsLittleEndian ? "ef cd 34 12" : "12 34 cd ef";
auto expectedRegex = auto expectedRegex =
R"(error formatting log message: test; )" R"(error formatting log message: .*test; )"
R"(format string: "param1: \{\}, param2: \{\}", )" R"(format string: "param1: \{\}, param2: \{\}", )"
R"(arguments: 1234, )" R"(arguments: 1234, )"
R"(\[(.*FormattableButNoToString.*|object) of size 4: )" + R"(\[(.*FormattableButNoToString.*|object) of size 4: )" +
...@@ -343,10 +343,11 @@ TEST_F(LoggerTest, logMacros) { ...@@ -343,10 +343,11 @@ TEST_F(LoggerTest, logMacros) {
// Bad format arguments should not throw // Bad format arguments should not throw
FB_LOGF(footest1234, ERR, "whoops: {}, {}", getValue()); FB_LOGF(footest1234, ERR, "whoops: {}, {}", getValue());
ASSERT_EQ(1, messages.size()); ASSERT_EQ(1, messages.size());
EXPECT_EQ( EXPECT_THAT(
R"(error formatting log message: argument index out of range; )" messages[0].first.getMessage(),
R"(format string: "whoops: {}, {}", arguments: 5)", MatchesRegex(
messages[0].first.getMessage()); R"(error formatting log message: .*argument index out of range; )"
R"(format string: "whoops: \{\}, \{\}", arguments: 5)"));
messages.clear(); messages.clear();
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment