Copilot commented on code in PR #13432:
URL: https://github.com/apache/trafficserver/pull/13432#discussion_r3661123602
##########
plugins/healthchecks/healthchecks.cc:
##########
@@ -53,66 +54,90 @@ typedef struct HCDirEntry_t {
struct HCDirEntry_t *_next; /* Linked list */
} HCDirEntry;
-/* Information about a status file. This is never modified (only replaced, see
HCFileInfo_t) */
-typedef struct HCFileData_t {
- int exists; /* Does this file exist */
- char body[MAX_BODY_LEN]; /* Body from fname. Empty string
means file is missing */
- int b_len; /* Length of data */
- time_t remove; /* Used for deciding when the old
object can be permanently removed */
- struct HCFileData_t *_next; /* Only used when these guys end up
on the freelist */
-} HCFileData;
-
-/* The only thing that should change in this struct is data, atomically
swapping ptrs */
-typedef struct HCFileInfo_t {
- char fname[MAX_PATH_LEN]; /* Filename */
- char *basename; /* The "basename" of the file
*/
- unsigned basename_len = 0; /* The length of the basename
*/
- char path[PATH_NAME_MAX]; /* URL path for this HC */
- int p_len; /* Length of path */
- const char *ok; /* Header for an OK result */
- int o_len; /* Length of OK header */
- const char *miss; /* Header for miss results */
- int m_len; /* Length of miss header */
- std::atomic<HCFileData *> data; /* Holds the current data for
this health check file */
- int wd; /* Watch descriptor */
- HCDirEntry *dir; /* Reference to the directory
this file resides in */
- struct HCFileInfo_t *_next; /* Linked list */
-} HCFileInfo;
+/* Information about a status file. This is never modified (only replaced, see
HCFileInfo) */
+struct HCFileData {
+ int exists = 0; /* Does this file exist */
+ int b_len = 0; /* Length of data */
+ char body[MAX_BODY_LEN] = {}; /* Body from fname. Empty string means file is
missing */
+};
+
+using HCFileDataPtr = std::shared_ptr<HCFileData>;
+
+/* The only thing that should change in this struct is data, which is replaced
(never modified) by
+ the inotify thread. Readers take a reference to the current data via
get_data(), which keeps
+ that snapshot alive for as long as the transaction needs it. */
+struct HCFileInfo {
+ char fname[MAX_PATH_LEN] = {}; /* Filename */
+ char *basename = nullptr; /* The "basename" of the file */
+ unsigned basename_len = 0; /* The length of the basename */
+ char path[PATH_NAME_MAX] = {}; /* URL path for this HC */
+ int p_len = 0; /* Length of path */
+ const char *ok = nullptr; /* Header for an OK result */
+ int o_len = 0; /* Length of OK header */
+ const char *miss = nullptr; /* Header for miss results */
+ int m_len = 0; /* Length of miss header */
+ int wd = 0; /* Watch descriptor */
+ HCDirEntry *dir = nullptr; /* Reference to the directory
this file resides in */
+ HCFileInfo *_next = nullptr; /* Linked list */
+
+ /* Take a reference to the current data for this health check file. */
+ HCFileDataPtr
+ get_data()
+ {
+ std::lock_guard<std::mutex> lock{_data_mutex};
+ return _data;
+ }
+
+ /* Replace the current data for this health check file. Snapshots handed out
by get_data() stay
+ valid until their last reference is dropped. */
+ void
+ set_data(HCFileDataPtr data)
+ {
+ std::lock_guard<std::mutex> lock{_data_mutex};
+ _data = std::move(data);
+ }
+
+private:
+ std::mutex _data_mutex; /* Protects @a _data */
+ HCFileDataPtr _data; /* Holds the current data for this health check
file */
+};
/* Global configuration */
HCFileInfo *g_config;
/* State used for the intercept plugin. ToDo: Can this be improved ? */
-typedef struct HCState_t {
- TSVConn net_vc;
- TSVIO read_vio;
- TSVIO write_vio;
+struct HCState {
+ TSVConn net_vc = nullptr;
+ TSVIO read_vio = nullptr;
+ TSVIO write_vio = nullptr;
- TSIOBuffer req_buffer;
- TSIOBuffer resp_buffer;
- TSIOBufferReader resp_reader;
+ TSIOBuffer req_buffer = nullptr;
+ TSIOBuffer resp_buffer = nullptr;
+ TSIOBufferReader resp_reader = nullptr;
- int output_bytes;
+ int output_bytes = 0;
- /* We actually need both here, so that our lock free switches works safely */
- HCFileInfo *info;
- HCFileData *data;
-} HCState;
+ /* We hold a reference to the data so that it cannot be replaced from under
us mid transaction */
+ HCFileInfo *info = nullptr;
+ HCFileDataPtr data;
+};
/* Read / check the status files */
-static void
-reload_status_file(HCFileInfo *info, HCFileData *data)
+static HCFileDataPtr
+load_status_file(HCFileInfo *info)
{
+ auto data = std::make_shared<HCFileData>();
FILE *fd;
- memset(data, 0, sizeof(HCFileData));
if (nullptr != (fd = fopen(info->fname, "r"))) {
data->exists = 1;
do {
data->b_len = fread(data->body, 1, MAX_BODY_LEN, fd);
} while (!feof(fd)); /* Only save the last 16KB of the file ... */
fclose(fd);
Review Comment:
The fread/feof do-while loop will perform one extra read after the final
full chunk. If the file size is an exact multiple of MAX_BODY_LEN, the final
fread returns 0, sets EOF, and overwrites b_len to 0, causing an empty response
body/content-length despite having read data. Consider switching to a loop that
updates b_len only on successful reads (>0).
##########
plugins/healthchecks/healthchecks.cc:
##########
@@ -53,66 +54,90 @@ typedef struct HCDirEntry_t {
struct HCDirEntry_t *_next; /* Linked list */
} HCDirEntry;
-/* Information about a status file. This is never modified (only replaced, see
HCFileInfo_t) */
-typedef struct HCFileData_t {
- int exists; /* Does this file exist */
- char body[MAX_BODY_LEN]; /* Body from fname. Empty string
means file is missing */
- int b_len; /* Length of data */
- time_t remove; /* Used for deciding when the old
object can be permanently removed */
- struct HCFileData_t *_next; /* Only used when these guys end up
on the freelist */
-} HCFileData;
-
-/* The only thing that should change in this struct is data, atomically
swapping ptrs */
-typedef struct HCFileInfo_t {
- char fname[MAX_PATH_LEN]; /* Filename */
- char *basename; /* The "basename" of the file
*/
- unsigned basename_len = 0; /* The length of the basename
*/
- char path[PATH_NAME_MAX]; /* URL path for this HC */
- int p_len; /* Length of path */
- const char *ok; /* Header for an OK result */
- int o_len; /* Length of OK header */
- const char *miss; /* Header for miss results */
- int m_len; /* Length of miss header */
- std::atomic<HCFileData *> data; /* Holds the current data for
this health check file */
- int wd; /* Watch descriptor */
- HCDirEntry *dir; /* Reference to the directory
this file resides in */
- struct HCFileInfo_t *_next; /* Linked list */
-} HCFileInfo;
+/* Information about a status file. This is never modified (only replaced, see
HCFileInfo) */
+struct HCFileData {
+ int exists = 0; /* Does this file exist */
+ int b_len = 0; /* Length of data */
+ char body[MAX_BODY_LEN] = {}; /* Body from fname. Empty string means file is
missing */
+};
+
+using HCFileDataPtr = std::shared_ptr<HCFileData>;
+
+/* The only thing that should change in this struct is data, which is replaced
(never modified) by
+ the inotify thread. Readers take a reference to the current data via
get_data(), which keeps
+ that snapshot alive for as long as the transaction needs it. */
+struct HCFileInfo {
+ char fname[MAX_PATH_LEN] = {}; /* Filename */
+ char *basename = nullptr; /* The "basename" of the file */
+ unsigned basename_len = 0; /* The length of the basename */
+ char path[PATH_NAME_MAX] = {}; /* URL path for this HC */
+ int p_len = 0; /* Length of path */
+ const char *ok = nullptr; /* Header for an OK result */
+ int o_len = 0; /* Length of OK header */
+ const char *miss = nullptr; /* Header for miss results */
+ int m_len = 0; /* Length of miss header */
+ int wd = 0; /* Watch descriptor */
+ HCDirEntry *dir = nullptr; /* Reference to the directory
this file resides in */
+ HCFileInfo *_next = nullptr; /* Linked list */
+
+ /* Take a reference to the current data for this health check file. */
+ HCFileDataPtr
+ get_data()
+ {
+ std::lock_guard<std::mutex> lock{_data_mutex};
+ return _data;
+ }
+
+ /* Replace the current data for this health check file. Snapshots handed out
by get_data() stay
+ valid until their last reference is dropped. */
+ void
+ set_data(HCFileDataPtr data)
+ {
+ std::lock_guard<std::mutex> lock{_data_mutex};
+ _data = std::move(data);
+ }
+
+private:
+ std::mutex _data_mutex; /* Protects @a _data */
+ HCFileDataPtr _data; /* Holds the current data for this health check
file */
+};
Review Comment:
HCFileInfo::get_data()/set_data() now take a std::mutex on every healthcheck
transaction. Since the previous implementation used an atomic swap, this
introduces avoidable contention/latency on what can be a hot path. Consider
using atomic shared_ptr operations (std::atomic_load/std::atomic_store on the
shared_ptr) to keep reads lock-free while still providing safe lifetime
semantics.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
