Fixed a recursive call to the async::processMessages#32853
Fixed a recursive call to the async::processMessages#32853igorkorsukov wants to merge 1 commit intomusescore:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a FluidSynth compile-time debug flag; tightened async callable construction/invocation; added a re-entrancy guard to RpcPort::process; switched deferred export scheduling to QTimer::singleShot; removed per-chunk async message pumping in soundtrack generation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/framework/global/thirdparty/kors_async/async/internal/rpcqueue.h (1)
103-114:⚠️ Potential issue | 🟠 MajorMake
m_isProcessingreset scope-bound.
m_isProcessingis cleared manually at Line 114. If control exits earlier while iterating handlers, the flag can staytrueand futureprocess()calls will keep returning early.Suggested fix
void process() { // try send pending sendPending(); assert(m_connPort); if (m_isProcessing) { return; } - assert(!m_isProcessing && "Recursive processing of messages is not allowed"); m_isProcessing = true; + struct ProcessingGuard { + bool& flag; + ~ProcessingGuard() { flag = false; } + } processingGuard { m_isProcessing }; // receive messages m_buffer.clear(); bool ok = m_connPort->m_queue.tryPopAll(m_buffer); if (ok && m_handler) { for (const T& item : m_buffer) { m_handler(item); } } - - m_isProcessing = false; }src/framework/global/thirdparty/kors_async/async/async.h (1)
115-122:⚠️ Potential issue | 🟠 MajorAdd runtime guards for callback validation instead of assert-only checks.
The assertions at lines 115 and 141 are stripped in release builds (NDEBUG is set in SetupBuildEnvironment.cmake). If an empty callback reaches line 146,
func()will crash without detection. While current usages always pass valid lambdas, the default parameter in the Func constructor permits invalid input. Add runtime validation to make invalid construction impossible and guard the call.Suggested fix
struct Func : public ICallable { Call func; - Func(const Call& f = nullptr) - : func(f) - { - assert(func && "callback is nullptr"); - } + explicit Func(const Call& f) + : func(f) {} void call(const void*) override { - func(); + if (func) { + func(); + } } }; + if (!func) { + return; + } + CallMsg m; m.receiver = caller; m.func = std::make_shared<Func>(func);Also applies to: 138-142, 146-146
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4017c81-bfc3-4d1d-a769-18b8a2d02f2f
📒 Files selected for processing (4)
src/framework/audio/engine/internal/synthesizers/fluidsynth/fluidsynth.cppsrc/framework/global/thirdparty/kors_async/async/async.hsrc/framework/global/thirdparty/kors_async/async/internal/rpcqueue.hsrc/project/qml/MuseScore/Project/internal/Export/exportdialogmodel.cpp
src/framework/global/thirdparty/kors_async/async/internal/rpcqueue.h
Outdated
Show resolved
Hide resolved
igorkorsukov
force-pushed
the
w/ioc/step_37
branch
from
3731683 to
3e9947d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/framework/audio/engine/internal/export/soundtrackwriter.cpp (1)
151-167: 🧹 Nitpick | 🔵 TrivialUnused variable and commented-out code should be cleaned up.
The variable
thisThIdon line 151 is now unused since its only reference is in the commented-out code on line 166. This will likely generate a compiler warning.If the
async::processMessagescall is intentionally being removed as part of this fix, consider removing both the commented code and the unused variable entirely rather than leaving dead code.♻️ Proposed cleanup
- const std::thread::id thisThId = std::this_thread::get_id(); while (inputBufferOffset < inputBufferMaxOffset && !m_isAborted) { m_source->process(m_intermBuffer.data(), m_renderStep); size_t samplesToCopy = std::min(m_intermBuffer.size(), inputBufferMaxOffset - inputBufferOffset); std::copy(m_intermBuffer.begin(), m_intermBuffer.begin() + samplesToCopy, m_inputBuffer.begin() + inputBufferOffset); inputBufferOffset += samplesToCopy; sendStepProgress(PREPARE_STEP, inputBufferOffset, inputBufferMaxOffset); //! NOTE It is necessary for cancellation to work //! and for information about the audio signal to be transmitted. - //async::processMessages(thisThId); rpcChannel()->process(); }src/framework/global/thirdparty/kors_async/async/internal/rpcqueue.h (2)
98-110:⚠️ Potential issue | 🟠 MajorRe-entrancy guard relies on assert which is disabled in release builds.
The
assert(!m_isProcessing)on line 98 only fires in debug builds. In release builds whereNDEBUGis defined,assert()is a no-op, meaning the function will still execute recursively without any protection.If re-entrancy truly should never happen (as the assertion message indicates), consider adding an early return after the assert to protect release builds as well:
🛡️ Proposed fix to protect release builds
assert(!m_isProcessing && "Recursive processing of messages is not allowed"); + if (m_isProcessing) { + return; + } m_isProcessing = true;
91-111:⚠️ Potential issue | 🟡 MinorGuard flag is not exception-safe.
If
m_handler(item)throws an exception (line 106),m_isProcessingwill remaintrueand never be reset, causing all subsequentprocess()calls to be blocked or skipped (depending on whether the early-return fix above is applied).Consider using RAII to ensure the flag is always reset:
🛡️ Proposed fix using RAII pattern
void process() { // try send pending sendPending(); assert(m_connPort); assert(!m_isProcessing && "Recursive processing of messages is not allowed"); + if (m_isProcessing) { + return; + } - m_isProcessing = true; + + struct Guard { + bool& flag; + Guard(bool& f) : flag(f) { flag = true; } + ~Guard() { flag = false; } + } guard(m_isProcessing); // receive messages m_buffer.clear(); bool ok = m_connPort->m_queue.tryPopAll(m_buffer); if (ok && m_handler) { for (const T& item : m_buffer) { m_handler(item); } } - - m_isProcessing = false; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 02f24880-a72a-4810-8927-8ac170712fcf
📒 Files selected for processing (5)
src/framework/audio/engine/internal/export/soundtrackwriter.cppsrc/framework/audio/engine/internal/synthesizers/fluidsynth/fluidsynth.cppsrc/framework/global/thirdparty/kors_async/async/async.hsrc/framework/global/thirdparty/kors_async/async/internal/rpcqueue.hsrc/project/qml/MuseScore/Project/internal/Export/exportdialogmodel.cpp
igorkorsukov
force-pushed
the
w/ioc/step_37
branch
2 times, most recently
from
3766dba to
f67a840
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17de9e1e-8cfa-4ac0-857c-b933b85af75a
📒 Files selected for processing (5)
src/framework/audio/engine/internal/export/soundtrackwriter.cppsrc/framework/audio/engine/internal/synthesizers/fluidsynth/fluidsynth.cppsrc/framework/global/thirdparty/kors_async/async/async.hsrc/framework/global/thirdparty/kors_async/async/internal/rpcqueue.hsrc/project/qml/MuseScore/Project/internal/Export/exportdialogmodel.cpp
💤 Files with no reviewable changes (1)
- src/framework/audio/engine/internal/export/soundtrackwriter.cpp
igorkorsukov
force-pushed
the
w/ioc/step_37
branch
from
f67a840 to
0337c8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78dee4d3-8bad-45e4-922f-e03f5ee35490
📒 Files selected for processing (5)
src/framework/audio/engine/internal/export/soundtrackwriter.cppsrc/framework/audio/engine/internal/synthesizers/fluidsynth/fluidsynth.cppsrc/framework/global/thirdparty/kors_async/async/async.hsrc/framework/global/thirdparty/kors_async/async/internal/rpcqueue.hsrc/project/qml/MuseScore/Project/internal/Export/exportdialogmodel.cpp
💤 Files with no reviewable changes (1)
- src/framework/audio/engine/internal/export/soundtrackwriter.cpp
igorkorsukov
force-pushed
the
w/ioc/step_37
branch
from
0337c8b to
4531610
Compare
2 participants
Resolves: #32819
Summary by CodeRabbit