You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@logging.apache.org by rm...@apache.org on 2022/12/17 22:05:18 UTC
[logging-log4cxx] 01/01: LOGCXX-568 Cleanup if GZip fails
This is an automated email from the ASF dual-hosted git repository.
rmiddleton pushed a commit to branch LOGCXX-568
in repository https://gitbox.apache.org/repos/asf/logging-log4cxx.git
commit 0e8d9d5e5471dab5f827337c8ef24395d5b1f059
Author: Robert Middleton <ro...@rm5248.com>
AuthorDate: Sat Dec 17 17:04:15 2022 -0500
LOGCXX-568 Cleanup if GZip fails
Cleanup 0-sized gzip files if rollover fails. Added a new option to determine
what to do if there is a failure, to make the rollover dumb.
---
src/main/cpp/file.cpp | 73 ++++++++++++++++------
src/main/cpp/fixedwindowrollingpolicy.cpp | 17 +++--
src/main/cpp/gzcompressaction.cpp | 32 +++++++++-
src/main/cpp/timebasedrollingpolicy.cpp | 24 ++++++-
src/main/cpp/zipcompressaction.cpp | 20 +++++-
src/main/include/log4cxx/file.h | 16 ++++-
.../include/log4cxx/rolling/gzcompressaction.h | 11 ++++
.../log4cxx/rolling/timebasedrollingpolicy.h | 3 +-
.../include/log4cxx/rolling/zipcompressaction.h | 11 ++++
9 files changed, 175 insertions(+), 32 deletions(-)
diff --git a/src/main/cpp/file.cpp b/src/main/cpp/file.cpp
index b5d09b87..56fe7682 100644
--- a/src/main/cpp/file.cpp
+++ b/src/main/cpp/file.cpp
@@ -27,7 +27,27 @@
using namespace log4cxx;
using namespace log4cxx::helpers;
-File::File()
+struct File::FilePrivate{
+ FilePrivate() :
+ autoDelete(false)
+ {}
+
+ FilePrivate(LogString path) :
+ path(path),
+ autoDelete(false)
+ {}
+
+ FilePrivate(LogString path, bool autoDelete) :
+ path(path),
+ autoDelete(autoDelete)
+ {}
+
+ LogString path;
+ bool autoDelete;
+};
+
+File::File() :
+ m_priv(std::make_unique<FilePrivate>())
{
}
@@ -54,48 +74,48 @@ static LogString decodeLS(const std::basic_string<S>& src)
File::File(const std::string& name1)
- : path(decodeLS(name1))
+ : m_priv(std::make_unique<FilePrivate>(decodeLS(name1)))
{
}
File::File(const char* name1)
- : path(decodeLS(name1))
+ : m_priv(std::make_unique<FilePrivate>(decodeLS(name1)))
{
}
#if LOG4CXX_WCHAR_T_API
File::File(const std::wstring& name1)
- : path(decodeLS(name1))
+ : m_priv(std::make_unique<FilePrivate>(decodeLS(name1)))
{
}
File::File(const wchar_t* name1)
- : path(decodeLS(name1))
+ : m_priv(std::make_unique<FilePrivate>(decodeLS(name1)))
{
}
#endif
#if LOG4CXX_UNICHAR_API
File::File(const std::basic_string<UniChar>& name1)
- : path(decodeLS(name1))
+ : m_priv(std::make_unique<FilePrivate>(decodeLS(name1)))
{
}
File::File(const UniChar* name1)
- : path(decodeLS(name1))
+ : m_priv(std::make_unique<FilePrivate>(decodeLS(name1)))
{
}
#endif
#if LOG4CXX_CFSTRING_API
File::File(const CFStringRef& name1)
- : path(decodeLS(name1))
+ : m_priv(std::make_unique<FilePrivate>(decodeLS(name1)))
{
}
#endif
File::File(const File& src)
- : path(src.path)
+ : m_priv(std::make_unique<FilePrivate>(src.m_priv->path, src.m_priv->autoDelete))
{
}
@@ -106,7 +126,8 @@ File& File::operator=(const File& src)
return *this;
}
- path.assign(src.path);
+ m_priv->path.assign(src.m_priv->path);
+ m_priv->autoDelete = src.m_priv->autoDelete;
return *this;
}
@@ -114,31 +135,35 @@ File& File::operator=(const File& src)
File::~File()
{
+ if(m_priv->autoDelete){
+ Pool p;
+ deleteFile(p);
+ }
}
LogString File::getPath() const
{
- return path;
+ return m_priv->path;
}
File& File::setPath(const LogString& newName)
{
- path.assign(newName);
+ m_priv->path.assign(newName);
return *this;
}
LogString File::getName() const
{
const logchar slashes[] = { 0x2F, 0x5C, 0 };
- size_t lastSlash = path.find_last_of(slashes);
+ size_t lastSlash = m_priv->path.find_last_of(slashes);
if (lastSlash != LogString::npos)
{
- return path.substr(lastSlash + 1);
+ return m_priv->path.substr(lastSlash + 1);
}
- return path;
+ return m_priv->path;
}
char* File::getPath(Pool& p) const
@@ -149,11 +174,11 @@ char* File::getPath(Pool& p) const
if (style == APR_FILEPATH_ENCODING_UTF8)
{
- retval = Transcoder::encodeUTF8(path, p);
+ retval = Transcoder::encodeUTF8(m_priv->path, p);
}
else
{
- retval = Transcoder::encode(path, p);
+ retval = Transcoder::encode(m_priv->path, p);
}
return retval;
@@ -279,8 +304,8 @@ std::vector<LogString> File::list(Pool& p) const
LogString File::getParent(Pool&) const
{
- LogString::size_type slashPos = path.rfind(LOG4CXX_STR('/'));
- LogString::size_type backPos = path.rfind(LOG4CXX_STR('\\'));
+ LogString::size_type slashPos = m_priv->path.rfind(LOG4CXX_STR('/'));
+ LogString::size_type backPos = m_priv->path.rfind(LOG4CXX_STR('\\'));
if (slashPos == LogString::npos)
{
@@ -298,7 +323,7 @@ LogString File::getParent(Pool&) const
if (slashPos != LogString::npos && slashPos > 0)
{
- parent.assign(path, 0, slashPos);
+ parent.assign(m_priv->path, 0, slashPos);
}
return parent;
@@ -310,3 +335,11 @@ bool File::mkdirs(Pool& p) const
APR_OS_DEFAULT, p.getAPRPool());
return stat == APR_SUCCESS;
}
+
+void File::setAutoDelete(bool autoDelete){
+ m_priv->autoDelete = autoDelete;
+}
+
+bool File::getAutoDelete() const{
+ return m_priv->autoDelete;
+}
diff --git a/src/main/cpp/fixedwindowrollingpolicy.cpp b/src/main/cpp/fixedwindowrollingpolicy.cpp
index 8534c5df..af4c55b6 100644
--- a/src/main/cpp/fixedwindowrollingpolicy.cpp
+++ b/src/main/cpp/fixedwindowrollingpolicy.cpp
@@ -47,6 +47,7 @@ struct FixedWindowRollingPolicy::FixedWindowRollingPolicyPrivate : public Rollin
int minIndex;
int maxIndex;
bool explicitActiveFile;
+ bool throwIOExceptionOnForkFailure = true;
};
IMPLEMENT_LOG4CXX_OBJECT(FixedWindowRollingPolicy)
@@ -68,8 +69,6 @@ void FixedWindowRollingPolicy::setMinIndex(int minIndex1)
priv->minIndex = minIndex1;
}
-
-
void FixedWindowRollingPolicy::setOption(const LogString& option,
const LogString& value)
{
@@ -85,6 +84,12 @@ void FixedWindowRollingPolicy::setOption(const LogString& option,
{
priv->maxIndex = OptionConverter::toInt(value, 7);
}
+ else if (StringHelper::equalsIgnoreCase(option,
+ LOG4CXX_STR("THROWIOEXCEPTIONONFORKFAILURE"),
+ LOG4CXX_STR("throwioexceptiononforkfailure")))
+ {
+ priv->throwIOExceptionOnForkFailure = OptionConverter::toBoolean(value, true);
+ }
else
{
RollingPolicyBase::setOption(option, value);
@@ -193,18 +198,22 @@ RolloverDescriptionPtr FixedWindowRollingPolicy::rollover(
if (StringHelper::endsWith(renameTo, LOG4CXX_STR(".gz")))
{
renameTo.resize(renameTo.size() - 3);
- compressAction = std::make_shared<GZCompressAction>(
+ GZCompressActionPtr comp = std::make_shared<GZCompressAction>(
File().setPath(renameTo),
File().setPath(compressedName),
true);
+ comp->setThrowIOExceptionOnForkFailure(priv->throwIOExceptionOnForkFailure);
+ compressAction = comp;
}
else if (StringHelper::endsWith(renameTo, LOG4CXX_STR(".zip")))
{
renameTo.resize(renameTo.size() - 4);
- compressAction = std::make_shared<ZipCompressAction>(
+ ZipCompressActionPtr comp = std::make_shared<ZipCompressAction>(
File().setPath(renameTo),
File().setPath(compressedName),
true);
+ comp->setThrowIOExceptionOnForkFailure(priv->throwIOExceptionOnForkFailure);
+ compressAction = comp;
}
auto renameAction = std::make_shared<FileRenameAction>(
diff --git a/src/main/cpp/gzcompressaction.cpp b/src/main/cpp/gzcompressaction.cpp
index f01d2a7f..c85331ad 100644
--- a/src/main/cpp/gzcompressaction.cpp
+++ b/src/main/cpp/gzcompressaction.cpp
@@ -21,6 +21,7 @@
#include <log4cxx/helpers/exception.h>
#include <log4cxx/helpers/transcoder.h>
#include <log4cxx/private/action_priv.h>
+#include <log4cxx/helpers/loglog.h>
using namespace log4cxx;
using namespace log4cxx::rolling;
@@ -36,8 +37,9 @@ struct GZCompressAction::GZCompressActionPrivate : public ActionPrivate
source(toRename), destination(renameTo), deleteSource(deleteSource) {}
const File source;
- const File destination;
+ File destination;
bool deleteSource;
+ bool throwIOExceptionOnForkFailure = true;
};
IMPLEMENT_LOG4CXX_OBJECT(GZCompressAction)
@@ -115,6 +117,8 @@ bool GZCompressAction::execute(log4cxx::helpers::Pool& p) const
}
}
+ priv->destination.setAutoDelete(true);
+
const char** args = (const char**)
apr_palloc(aprpool, 4 * sizeof(*args));
int i = 0;
@@ -123,13 +127,29 @@ bool GZCompressAction::execute(log4cxx::helpers::Pool& p) const
args[i++] = Transcoder::encode(priv->source.getPath(), p);
args[i++] = NULL;
-
apr_proc_t pid;
stat = apr_proc_create(&pid, "gzip", args, NULL, attr, aprpool);
- if (stat != APR_SUCCESS)
+ if (stat != APR_SUCCESS && priv->throwIOExceptionOnForkFailure)
{
throw IOException(stat);
+ }else if(stat != APR_SUCCESS && !priv->throwIOExceptionOnForkFailure)
+ {
+ /* If we fail here (to create the gzip child process),
+ * skip the compression and consider the rotation to be
+ * otherwise successful. The caller has already rotated
+ * the log file (`source` here refers to the
+ * uncompressed, rotated path, and `destination` the
+ * same path with `.gz` appended). Remove the empty
+ * destination file and leave source as-is.
+ */
+ LogLog::warn(LOG4CXX_STR("Failed to fork gzip during log rotation; leaving log file uncompressed"));
+ stat = apr_file_close(child_out);
+ if (stat != APR_SUCCESS)
+ {
+ LogLog::warn(LOG4CXX_STR("Failed to close abandoned .gz file; ignoring"));
+ }
+ return true;
}
apr_proc_wait(&pid, NULL, NULL, APR_WAIT);
@@ -140,6 +160,8 @@ bool GZCompressAction::execute(log4cxx::helpers::Pool& p) const
throw IOException(stat);
}
+ priv->destination.setAutoDelete(false);
+
if (priv->deleteSource)
{
priv->source.deleteFile(p);
@@ -151,3 +173,7 @@ bool GZCompressAction::execute(log4cxx::helpers::Pool& p) const
return false;
}
+void GZCompressAction::setThrowIOExceptionOnForkFailure(bool throwIO){
+ priv->throwIOExceptionOnForkFailure = throwIO;
+}
+
diff --git a/src/main/cpp/timebasedrollingpolicy.cpp b/src/main/cpp/timebasedrollingpolicy.cpp
index ed63f21b..eb1cbc91 100644
--- a/src/main/cpp/timebasedrollingpolicy.cpp
+++ b/src/main/cpp/timebasedrollingpolicy.cpp
@@ -112,6 +112,7 @@ struct TimeBasedRollingPolicy::TimeBasedRollingPolicyPrivate{
LogString _fileNamePattern;
bool multiprocess = false;
+ bool throwIOExceptionOnForkFailure = true;
};
@@ -453,14 +454,18 @@ RolloverDescriptionPtr TimeBasedRollingPolicy::rollover(
if (m_priv->suffixLength == 3)
{
- compressAction = std::make_shared<GZCompressAction>(
+ GZCompressActionPtr comp = std::make_shared<GZCompressAction>(
File().setPath(lastBaseName), File().setPath(m_priv->lastFileName), true);
+ comp->setThrowIOExceptionOnForkFailure(m_priv->throwIOExceptionOnForkFailure);
+ compressAction = comp;
}
if (m_priv->suffixLength == 4)
{
- compressAction = std::make_shared<ZipCompressAction>(
+ ZipCompressActionPtr comp = std::make_shared<ZipCompressAction>(
File().setPath(lastBaseName), File().setPath(m_priv->lastFileName), true);
+ comp->setThrowIOExceptionOnForkFailure(m_priv->throwIOExceptionOnForkFailure);
+ compressAction = comp;
}
if( m_priv->multiprocess ){
@@ -522,3 +527,18 @@ void TimeBasedRollingPolicy::setMultiprocess(bool multiprocess){
m_priv->multiprocess = multiprocess;
#endif
}
+
+void TimeBasedRollingPolicy::setOption(const LogString& option,
+ const LogString& value)
+{
+ if (StringHelper::equalsIgnoreCase(option,
+ LOG4CXX_STR("THROWIOEXCEPTIONONFORKFAILURE"),
+ LOG4CXX_STR("throwioexceptiononforkfailure")))
+ {
+ m_priv->throwIOExceptionOnForkFailure = OptionConverter::toBoolean(value, true);
+ }
+ else
+ {
+ RollingPolicyBase::setOption(option, value);
+ }
+}
diff --git a/src/main/cpp/zipcompressaction.cpp b/src/main/cpp/zipcompressaction.cpp
index 75206751..fa19b8e3 100644
--- a/src/main/cpp/zipcompressaction.cpp
+++ b/src/main/cpp/zipcompressaction.cpp
@@ -21,6 +21,7 @@
#include <log4cxx/helpers/exception.h>
#include <log4cxx/helpers/transcoder.h>
#include <log4cxx/private/action_priv.h>
+#include <log4cxx/helpers/loglog.h>
using namespace log4cxx;
using namespace log4cxx::rolling;
@@ -38,6 +39,7 @@ struct ZipCompressAction::ZipCompressActionPrivate : public ActionPrivate
const File source;
const File destination;
bool deleteSource;
+ bool throwIOExceptionOnForkFailure = true;
};
IMPLEMENT_LOG4CXX_OBJECT(ZipCompressAction)
@@ -114,9 +116,21 @@ bool ZipCompressAction::execute(log4cxx::helpers::Pool& p) const
apr_proc_t pid;
stat = apr_proc_create(&pid, "zip", args, NULL, attr, aprpool);
- if (stat != APR_SUCCESS)
+ if (stat != APR_SUCCESS && priv->throwIOExceptionOnForkFailure)
{
throw IOException(stat);
+ }else if(stat != APR_SUCCESS && !priv->throwIOExceptionOnForkFailure)
+ {
+ /* If we fail here (to create the zip child process),
+ * skip the compression and consider the rotation to be
+ * otherwise successful. The caller has already rotated
+ * the log file (`source` here refers to the
+ * uncompressed, rotated path, and `destination` the
+ * same path with `.zip` appended). Remove the empty
+ * destination file and leave source as-is.
+ */
+ LogLog::warn(LOG4CXX_STR("Failed to fork zip during log rotation; leaving log file uncompressed"));
+ return true;
}
int exitCode;
@@ -134,3 +148,7 @@ bool ZipCompressAction::execute(log4cxx::helpers::Pool& p) const
return true;
}
+
+void ZipCompressAction::setThrowIOExceptionOnForkFailure(bool throwIO){
+ priv->throwIOExceptionOnForkFailure = throwIO;
+}
diff --git a/src/main/include/log4cxx/file.h b/src/main/include/log4cxx/file.h
index 4d4ead8c..cc669eaf 100644
--- a/src/main/include/log4cxx/file.h
+++ b/src/main/include/log4cxx/file.h
@@ -176,8 +176,22 @@ class LOG4CXX_EXPORT File
*/
bool mkdirs(log4cxx::helpers::Pool& p) const;
+ /**
+ * Set the file to be deleted when this object is destroyed.
+ * @param autoDelete If true, delete file upon destruction. If true, do not delete file.
+ */
+ void setAutoDelete(bool autoDelete);
+
+ /**
+ * Return the value of the autodelete setting. If true, this file will be deleted when the
+ * destructor is called.
+ *
+ * @return True if the file is deleted upon destruction.
+ */
+ bool getAutoDelete() const;
+
private:
- LOG4CXX_DECLARE_PRIVATE_MEMBER(LogString, path)
+ LOG4CXX_DECLARE_PRIVATE_MEMBER_PTR(FilePrivate, m_priv)
static char* convertBackSlashes(char*);
char* getPath(log4cxx::helpers::Pool& p) const;
};
diff --git a/src/main/include/log4cxx/rolling/gzcompressaction.h b/src/main/include/log4cxx/rolling/gzcompressaction.h
index ef01cd47..f014003a 100644
--- a/src/main/include/log4cxx/rolling/gzcompressaction.h
+++ b/src/main/include/log4cxx/rolling/gzcompressaction.h
@@ -52,6 +52,17 @@ class GZCompressAction : public Action
*/
bool execute(log4cxx::helpers::Pool& pool) const override;
+ /**
+ * Set to true to throw an IOException on a fork failure. By default, this
+ * is true. When an IOException is thrown, this will automatically cause the
+ * error handler to be called(which is the recommended way of handling this
+ * problem). By setting this to true, the GZCompressAction effectively
+ * turns into a FileRenameAction if any errors are encountered.
+ *
+ * @param throwIO
+ */
+ void setThrowIOExceptionOnForkFailure(bool throwIO);
+
private:
GZCompressAction(const GZCompressAction&);
GZCompressAction& operator=(const GZCompressAction&);
diff --git a/src/main/include/log4cxx/rolling/timebasedrollingpolicy.h b/src/main/include/log4cxx/rolling/timebasedrollingpolicy.h
index cdbe65bc..b1cfd221 100755
--- a/src/main/include/log4cxx/rolling/timebasedrollingpolicy.h
+++ b/src/main/include/log4cxx/rolling/timebasedrollingpolicy.h
@@ -190,7 +190,8 @@ class LOG4CXX_EXPORT TimeBasedRollingPolicy : public virtual RollingPolicyBase,
const LogString& filename,
size_t fileLength) override;
- using RollingPolicyBase::setOption;
+ void setOption(const LogString& option, const LogString& value) override;
+
protected:
log4cxx::pattern::PatternMap getFormatSpecifiers() const override;
diff --git a/src/main/include/log4cxx/rolling/zipcompressaction.h b/src/main/include/log4cxx/rolling/zipcompressaction.h
index 126cf06a..563f4ea5 100644
--- a/src/main/include/log4cxx/rolling/zipcompressaction.h
+++ b/src/main/include/log4cxx/rolling/zipcompressaction.h
@@ -51,6 +51,17 @@ class ZipCompressAction : public Action
*/
bool execute(log4cxx::helpers::Pool& pool) const override;
+ /**
+ * Set to true to throw an IOException on a fork failure. By default, this
+ * is true. When an IOException is thrown, this will automatically cause the
+ * error handler to be called(which is the recommended way of handling this
+ * problem). By setting this to true, the ZipCompressAction effectively
+ * turns into a FileRenameAction if any errors are encountered.
+ *
+ * @param throwIO
+ */
+ void setThrowIOExceptionOnForkFailure(bool throwIO);
+
private:
ZipCompressAction(const ZipCompressAction&);
ZipCompressAction& operator=(const ZipCompressAction&);