You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by rr...@apache.org on 2020/06/02 14:45:12 UTC
[trafficserver] branch master updated: traffic_dump: debug_tag and
lock improvements
This is an automated email from the ASF dual-hosted git repository.
rrm pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 8e59ed0 traffic_dump: debug_tag and lock improvements
8e59ed0 is described below
commit 8e59ed0636c6eee2d4f77f1cb1b31766e1c71ede
Author: bneradt <bn...@verizonmedia.com>
AuthorDate: Wed May 27 21:18:42 2020 +0000
traffic_dump: debug_tag and lock improvements
These are changes from a set of suggestions that Walt Karas made
internally. Incidentally, these changes to disk_io_mutex likely
addresses a crash we saw for the mutex when it was acccidentally double
freed.
---
.../experimental/traffic_dump/global_variables.h | 2 +-
plugins/experimental/traffic_dump/session_data.cc | 30 ++++++++--------------
plugins/experimental/traffic_dump/session_data.h | 10 +++++---
3 files changed, 18 insertions(+), 24 deletions(-)
diff --git a/plugins/experimental/traffic_dump/global_variables.h b/plugins/experimental/traffic_dump/global_variables.h
index 204ce20..18a9555 100644
--- a/plugins/experimental/traffic_dump/global_variables.h
+++ b/plugins/experimental/traffic_dump/global_variables.h
@@ -20,6 +20,6 @@
namespace traffic_dump
{
-char const constexpr *const debug_tag = "traffic_dump";
+constexpr char debug_tag[] = "traffic_dump";
} // namespace traffic_dump
diff --git a/plugins/experimental/traffic_dump/session_data.cc b/plugins/experimental/traffic_dump/session_data.cc
index ffbdb50..e77c788 100644
--- a/plugins/experimental/traffic_dump/session_data.cc
+++ b/plugins/experimental/traffic_dump/session_data.cc
@@ -232,16 +232,12 @@ SessionData::get_client_protocol_description(TSHttpSsn ssnp)
SessionData::SessionData()
{
- disk_io_mutex = TSMutexCreate();
- aio_cont = TSContCreate(session_aio_handler, TSMutexCreate());
- txn_cont = TSContCreate(TransactionData::global_transaction_handler, nullptr);
+ aio_cont = TSContCreate(session_aio_handler, TSMutexCreate());
+ txn_cont = TSContCreate(TransactionData::global_transaction_handler, nullptr);
}
SessionData::~SessionData()
{
- if (disk_io_mutex) {
- TSMutexDestroy(disk_io_mutex);
- }
if (aio_cont) {
TSContDestroy(aio_cont);
}
@@ -267,7 +263,6 @@ SessionData::write_to_disk_no_lock(std::string_view content)
write_offset += content.size();
aio_count += 1;
- TSMutexUnlock(disk_io_mutex);
return TS_SUCCESS;
}
TSfree(pBuf);
@@ -278,16 +273,15 @@ SessionData::write_to_disk_no_lock(std::string_view content)
int
SessionData::write_to_disk(std::string_view content)
{
- TSMutexLock(disk_io_mutex);
+ const std::lock_guard<std::recursive_mutex> _(disk_io_mutex);
const int result = write_to_disk_no_lock(content);
- TSMutexUnlock(disk_io_mutex);
return result;
}
int
SessionData::write_transaction_to_disk(std::string_view content)
{
- TSMutexLock(disk_io_mutex);
+ const std::lock_guard<std::recursive_mutex> _(disk_io_mutex);
int result = TS_SUCCESS;
if (has_written_first_transaction) {
@@ -302,7 +296,6 @@ SessionData::write_transaction_to_disk(std::string_view content)
has_written_first_transaction = true;
}
- TSMutexUnlock(disk_io_mutex);
return result;
}
@@ -318,7 +311,7 @@ SessionData::session_aio_handler(TSCont contp, TSEvent event, void *edata)
return TS_ERROR;
}
char *buf = TSAIOBufGet(cb);
- TSMutexLock(ssnData->disk_io_mutex);
+ const std::lock_guard<std::recursive_mutex> _(ssnData->disk_io_mutex);
// Free the allocated buffer and update aio_count
if (buf) {
@@ -327,7 +320,6 @@ SessionData::session_aio_handler(TSCont contp, TSEvent event, void *edata)
// check for ssn close, if closed, do clean up
TSContDataSet(contp, nullptr);
close(ssnData->log_fd);
- TSMutexUnlock(ssnData->disk_io_mutex);
std::error_code ec;
ts::file::file_status st = ts::file::status(ssnData->log_name, ec);
if (!ec) {
@@ -338,7 +330,6 @@ SessionData::session_aio_handler(TSCont contp, TSEvent event, void *edata)
return TS_SUCCESS;
}
}
- TSMutexUnlock(ssnData->disk_io_mutex);
return TS_SUCCESS;
}
default:
@@ -426,7 +417,7 @@ SessionData::global_session_handler(TSCont contp, TSEvent event, void *edata)
}
// Initialize AIO file
- TSMutexLock(ssnData->disk_io_mutex);
+ const std::lock_guard<std::recursive_mutex> _(ssnData->disk_io_mutex);
if (ssnData->log_fd < 0) {
ts::file::path log_p = log_directory / ts::file::path(std::string(client_str, 3));
ts::file::path log_f = log_p / ts::file::path(session_hex_name);
@@ -442,7 +433,6 @@ SessionData::global_session_handler(TSCont contp, TSEvent event, void *edata)
// Try to open log files for AIO
ssnData->log_fd = open(log_f.c_str(), O_RDWR | O_CREAT, S_IRWXU);
if (ssnData->log_fd < 0) {
- TSMutexUnlock(ssnData->disk_io_mutex);
TSDebug(debug_tag, "global_session_handler(): Failed to open log files %s. Abort.", log_f.c_str());
TSHttpSsnReenable(ssnp, TS_EVENT_HTTP_CONTINUE);
return TS_EVENT_HTTP_CONTINUE;
@@ -451,7 +441,6 @@ SessionData::global_session_handler(TSCont contp, TSEvent event, void *edata)
// Write log file beginning to disk
ssnData->write_to_disk(beginning);
}
- TSMutexUnlock(ssnData->disk_io_mutex);
TSHttpSsnHookAdd(ssnp, TS_HTTP_TXN_START_HOOK, ssnData->txn_cont);
TSHttpSsnHookAdd(ssnp, TS_HTTP_TXN_CLOSE_HOOK, ssnData->txn_cont);
@@ -470,9 +459,10 @@ SessionData::global_session_handler(TSCont contp, TSEvent event, void *edata)
return TS_SUCCESS;
}
ssnData->write_to_disk(json_closing);
- TSMutexLock(ssnData->disk_io_mutex);
- ssnData->ssn_closed = true;
- TSMutexUnlock(ssnData->disk_io_mutex);
+ {
+ const std::lock_guard<std::recursive_mutex> _(ssnData->disk_io_mutex);
+ ssnData->ssn_closed = true;
+ }
break;
}
diff --git a/plugins/experimental/traffic_dump/session_data.h b/plugins/experimental/traffic_dump/session_data.h
index 0094e72..7551501 100644
--- a/plugins/experimental/traffic_dump/session_data.h
+++ b/plugins/experimental/traffic_dump/session_data.h
@@ -20,6 +20,7 @@
#include <atomic>
#include <cstdlib>
+#include <mutex>
#include <string_view>
#include "ts/ts.h"
@@ -61,9 +62,12 @@ private:
/// Whether the first transaction in this session has been written.
bool has_written_first_transaction = false;
- TSCont aio_cont = nullptr; /// AIO continuation callback
- TSCont txn_cont = nullptr; /// Transaction continuation callback
- TSMutex disk_io_mutex = nullptr; /// AIO mutex
+ TSCont aio_cont = nullptr; /// AIO continuation callback
+ TSCont txn_cont = nullptr; /// Transaction continuation callback
+
+ // The following has to be recursive because the stack does not unwind
+ // between event invocations.
+ std::recursive_mutex disk_io_mutex; /// The mutex for guarding IO calls.
//
// Static Variables