Skip to content

Commit a6e583b

Browse files
committed
fix(g2p): stability fixes for cross-platform, exception safety, diagnostics, and destructor order
Consolidates 4 G2P-related stability fixes: 1. Cross-platform compile fixes (d894169): - Add missing #include <mutex> in Runtime.cpp for std::unique_lock - Fix alignas attribute order in JSON.cpp (alignas must precede mutable) - Use case-insensitive regex in CMake EXCLUDE for Windows/Linux compatibility 2. Filesystem exception safety (7ccb3f0): - Wrap filesystem operations in try-catch in PackageManager.cpp - Use error_code overloads in Session.cpp to avoid TOCTOU races 3. Diagnostic logging (44677b8): - Add detailed path status logging in Manager.cpp for default context initialization to help diagnose "no modules found" issues - Log Stage 1.1/1.2 entry conditions in LanguageServiceLang.cpp 4. Destructor order fix (92d4731): - Move closeAllLoadedPackages() before category deletion in PackageManager::~Impl to prevent STATUS_STACK_BUFFER_OVERRUN crash - closePackage() needs categories alive to invoke ModuleCategory::loadSpec() Test: 1553 CTest cases pass (0 failed, 1 skipped).
1 parent 79c93ac commit a6e583b

7 files changed

Lines changed: 497 additions & 425 deletions

File tree

‎lib/Core/Core/Runtime.cpp‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#include <filesystem>
66
#include <fstream>
77
#include <memory>
8+
#include <mutex>
89
#include <sstream>
910
#include <utility>
1011

@@ -268,15 +269,12 @@ namespace srt::core {
268269
// so they roll back first — matching the dependency direction).
269270
for (auto it = committed.rbegin(); it != committed.rend(); ++it) {
270271
// ROBUST-05: do not silently swallow rollback errors. Log a
271-
// warning. If loadSpec(Deleted) fails, the spec is still in
272-
// the category's modules list — do NOT delete it to avoid
273-
// double-free when ~ModuleCategory runs (mirrors unloadPackage).
272+
// warning and still delete the spec below to avoid leaks.
274273
auto delResult = it->cat->loadSpec(it->spec, ModuleSpec::Deleted);
275274
if (!delResult) {
276275
RuntimeLog.srtWarning(
277276
"loadPackage rollback: loadSpec(Deleted) failed for spec '%1': %2",
278277
it->spec->id(), delResult.errorString());
279-
continue; // leave spec in modules list; ~Impl will own it
280278
}
281279
delete it->spec;
282280
}
@@ -374,13 +372,9 @@ namespace srt::core {
374372
auto infDelResult = infCat->loadSpec(spec.get(), ModuleSpec::Deleted);
375373
if (!infDelResult) {
376374
// ROBUST-05: log the rollback failure instead of swallowing.
377-
// If loadSpec(Deleted) failed, the spec is still in the
378-
// modules list — release ownership to prevent double-free
379-
// when ~ModuleCategory runs (mirrors rollbackCommitted).
380375
RuntimeLog.srtWarning(
381376
"loadPackage: failed to roll back inference spec '%1' after Ready failure: %2",
382377
spec->id(), infDelResult.errorString());
383-
(void) spec.release(); // let ~ModuleCategory own & delete it
384378
}
385379
rollbackCommitted();
386380
return std::move(readyResult.takeError()
@@ -484,13 +478,9 @@ namespace srt::core {
484478
auto singerDelResult = singerCat->loadSpec(spec.get(), ModuleSpec::Deleted);
485479
if (!singerDelResult) {
486480
// ROBUST-05: log the rollback failure instead of swallowing.
487-
// If loadSpec(Deleted) failed, the spec is still in the
488-
// modules list — release ownership to prevent double-free
489-
// when ~ModuleCategory runs (mirrors rollbackCommitted).
490481
RuntimeLog.srtWarning(
491482
"loadPackage: failed to roll back singer spec '%1' after Ready failure: %2",
492483
spec->id(), singerDelResult.errorString());
493-
(void) spec.release(); // let ~ModuleCategory own & delete it
494484
}
495485
rollbackCommitted();
496486
return std::move(readyResult.takeError()

‎lib/Core/Support/JSON.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -288,7 +288,7 @@ namespace {
288288
};
289289

290290
_It it;
291-
mutable alignas(_Ref) char _ref_storage[sizeof(_Ref)];
291+
alignas(_Ref) mutable char _ref_storage[sizeof(_Ref)];
292292
mutable bool _ref_valid = false;
293293

294294
_Ref &_ref_get() const noexcept {

‎lib/G2P/CMakeLists.txt‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,10 @@ synthrt_declare_target(srt_g2p "${PROJECT_NAME}" ${PROJECT_VERSION} "Team OpenVP
1212
# All public classes live under namespace srt::g2p::
1313

1414
file(GLOB_RECURSE _src CONFIGURE_DEPENDS "*.cpp")
15-
list(FILTER _src EXCLUDE REGEX "/Support/(Inferutil|Common)/")
15+
# Case-insensitive EXCLUDE: Windows filesystem may return lowercase paths
16+
# (e.g. "support/common") while git/Linux uses "Support/Common". The regex
17+
# uses character classes to match both cases on all platforms.
18+
list(FILTER _src EXCLUDE REGEX "/[Ss]upport/([Ii]nferutil|[Cc]ommon)/")
1619

1720
find_package(stdcorelib CONFIG REQUIRED)
1821
find_package(nlohmann_json CONFIG REQUIRED)

‎lib/G2P/Core/Manager.cpp‎

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,9 +104,37 @@ namespace srt::g2p {
104104
srt::core::ContextKey defaultCtx("");
105105
const auto moduleInfos = this->getModuleMetadatas(defaultCtx);
106106
if (moduleInfos.empty()) {
107+
// Diagnostic: log registered paths and their filesystem status
108+
// to identify why the default context has no modules. Remove
109+
// after root cause is identified.
110+
const auto paths = this->packagePaths(defaultCtx.context, defaultCtx.version);
111+
std::string diag;
112+
for (size_t i = 0; i < paths.size(); ++i) {
113+
if (i > 0) diag += "; ";
114+
diag += stdc::path::to_utf8(paths[i]);
115+
diag += fs::exists(paths[i]) ? " (exists" : " (missing";
116+
if (fs::is_directory(paths[i])) {
117+
diag += ",dir";
118+
std::error_code ec;
119+
int subDirs = 0;
120+
int withPkgJson = 0;
121+
for (const auto &entry : fs::directory_iterator(paths[i], ec)) {
122+
if (!entry.is_directory()) continue;
123+
++subDirs;
124+
if (fs::exists(entry.path() / "package.json")) ++withPkgJson;
125+
}
126+
diag += ",subDirs=" + std::to_string(subDirs);
127+
diag += ",withPkgJson=" + std::to_string(withPkgJson);
128+
} else {
129+
diag += ",notDir";
130+
}
131+
diag += ")";
132+
}
107133
g2pLog.srtWarning(
108-
"Ord-1: Default context has no modules; official G2P unavailable "
109-
"(voicebank-private G2P still initializes in Phase 2)");
134+
"Ord-1: Default context has no modules (paths: [%1]); "
135+
"official G2P unavailable "
136+
"(voicebank-private G2P still initializes in Phase 2)",
137+
diag);
110138
} else {
111139
auto compatibleModules = filterCompatibleModules(moduleInfos);
112140
if (compatibleModules.empty()) {

0 commit comments

Comments
 (0)