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:17 UTC
[logging-log4cxx] branch LOGCXX-568 created (now 0e8d9d5e)
This is an automated email from the ASF dual-hosted git repository.
rmiddleton pushed a change to branch LOGCXX-568
in repository https://gitbox.apache.org/repos/asf/logging-log4cxx.git
at 0e8d9d5e LOGCXX-568 Cleanup if GZip fails
This branch includes the following new commits:
new 0e8d9d5e LOGCXX-568 Cleanup if GZip fails
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
[logging-log4cxx] 01/01: LOGCXX-568 Cleanup if GZip fails
Posted by rm...@apache.org.
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&);