Skip to content

Commit 8183091

Browse files
committed
Access log suppression cvars less thread-unsafely
In the engine, access the logs.suppression.* cvars through their objects, instead of looking them up in the cvar map via their names. Looking them up via name is seriously unsafe on a non-main thread. Simply reading the int or bool is still technically thread-unsafe, but should be fine in practice. The gamelogic still has to look up cvars by name, but there is no issue because logging is prohibited on non-main threads. Fixes, for practical purposes, #21.
1 parent dcdbc69 commit 8183091

File tree

2 files changed

+29
-23
lines changed

2 files changed

+29
-23
lines changed

src/common/Log.cpp

Lines changed: 29 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,31 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
3030

3131
#include "Common.h"
3232

33+
#ifdef BUILD_ENGINE
34+
// TODO: when cvar multiple registration is available, just declare the cvars in each module.
35+
static Cvar::Cvar<bool> suppressionEnabled(
36+
"logs.suppression.enabled", "Whether to suppress log messages that are printed too many times", Cvar::NONE, true);
37+
static Cvar::Range<Cvar::Cvar<int>> suppressionInterval(
38+
"logs.suppression.interval", "Interval in milliseconds for detecting log spam", Cvar::NONE, 2000, 1, 1000000);
39+
static Cvar::Range<Cvar::Cvar<int>> suppressionCount(
40+
"logs.suppression.count", "Number of occurrences for a message to be considered log spam", Cvar::NONE, 10, 1, 1000000);
41+
static Cvar::Range<Cvar::Cvar<int>> suppressionBufSize(
42+
"logs.suppression.bufferSize", "How many distinct messages to track for log suppression", Cvar::NONE, 50, 1, 1000000);
43+
44+
#define GET_LOG_CVAR(type, name, object) (object.Get())
45+
#else
46+
#define GET_LOG_CVAR(type, name, object) (GetCvarOrDie<type>(name))
47+
template <typename T>
48+
static T GetCvarOrDie(Str::StringRef cvar) {
49+
T value;
50+
std::string valueString = Cvar::GetValue(cvar);
51+
if (!Cvar::ParseCvarValue(valueString, value)) {
52+
Sys::Error("Failed to deserialize cvar %s with value: %s", cvar, valueString);
53+
}
54+
return value;
55+
}
56+
#endif
57+
3358
namespace Log {
3459
Cvar::Cvar<bool> logExtendAll(
3560
"logs.writeSrcLocation.all", "Always print source code location for logs", Cvar::NONE, false );
@@ -137,15 +162,6 @@ namespace Log {
137162
}
138163
}
139164

140-
template <typename T>
141-
static T GetCvarOrDie(Str::StringRef cvar) {
142-
T value;
143-
std::string valueString = Cvar::GetValue(cvar);
144-
if (!Cvar::ParseCvarValue(valueString, value)) {
145-
Sys::Error("Failed to deserialize cvar %s with value: %s", cvar, valueString);
146-
}
147-
return value;
148-
}
149165
namespace {
150166
// Log-spam suppression: if more than MAX_OCCURRENCES log messages with the same format string
151167
// are sent in less than INTERVAL_MS milliseconds, they will stop being printed.
@@ -167,9 +183,9 @@ namespace Log {
167183
};
168184
Result UpdateAndEvaluate(Str::StringRef messageFormat) {
169185
std::lock_guard<std::mutex> lock(mutex);
170-
int intervalMs = GetCvarOrDie<int>("logs.suppression.interval");
171-
int maxOccurrences = GetCvarOrDie<int>("logs.suppression.count");
172-
int bufferSize = GetCvarOrDie<int>("logs.suppression.bufferSize");
186+
int intervalMs = GET_LOG_CVAR(int, "logs.suppression.interval", suppressionInterval);
187+
int maxOccurrences = GET_LOG_CVAR(int, "logs.suppression.count", suppressionCount);
188+
int bufferSize = GET_LOG_CVAR(int, "logs.suppression.bufferSize", suppressionBufSize);
173189
buf.resize(bufferSize);
174190
MessageStatistics* oldest = &buf[0];
175191
int now = Sys::Milliseconds();
@@ -200,7 +216,7 @@ namespace Log {
200216

201217
void DispatchWithSuppression(std::string message, Log::Level level, Str::StringRef format) {
202218
static LogSpamSuppressor suppressor;
203-
if (level == Level::DEBUG || !GetCvarOrDie<bool>("logs.suppression.enabled")) {
219+
if (level == Level::DEBUG || !GET_LOG_CVAR(bool, "logs.suppression.enabled", suppressionEnabled)) {
204220
DispatchByLevel(std::move(message), level);
205221
return;
206222
}

src/engine/framework/LogSystem.cpp

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -34,16 +34,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
3434
#include "LogSystem.h"
3535

3636
namespace Log {
37-
38-
static Cvar::Cvar<bool> suppressionEnabled(
39-
"logs.suppression.enabled", "Whether to suppress log messages that are printed too many times", Cvar::NONE, true);
40-
static Cvar::Range<Cvar::Cvar<int>> suppressionInterval(
41-
"logs.suppression.interval", "Interval in milliseconds for detecting log spam", Cvar::NONE, 2000, 1, 1000000);
42-
static Cvar::Range<Cvar::Cvar<int>> suppressionCount(
43-
"logs.suppression.count", "Number of occurrences for a message to be considered log spam", Cvar::NONE, 10, 1, 1000000);
44-
static Cvar::Range<Cvar::Cvar<int>> suppressionBufSize(
45-
"logs.suppression.bufferSize", "How many distinct messages to track for log suppression", Cvar::NONE, 50, 1, 1000000);
46-
4737
static Target* targets[MAX_TARGET_ID];
4838

4939

0 commit comments

Comments
 (0)