Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stack overflow if AdmPresetDefinitionsHelper first inited on NNG thread #277

Closed
firthm01 opened this issue May 4, 2024 · 1 comment · Fixed by #278
Closed

Stack overflow if AdmPresetDefinitionsHelper first inited on NNG thread #277

firthm01 opened this issue May 4, 2024 · 1 comment · Fixed by #278
Labels
bug Something isn't working

Comments

@firthm01
Copy link
Collaborator

firthm01 commented May 4, 2024

The NNG threads have limited stack, and if AdmPresetDefinitionsHelper::getSingleton() is first called via an NNG message, it will overflow. This is because construction of the singleton runs a bunch of libadm stuff to parse the common definitions. The crash was observed in the binaural monitoring plugin.

Call stack:

 	EAR Binaural Monitoring.vst3!__chkstk() Line 109	
 	EAR Binaural Monitoring.vst3!adm::xml::XmlParser::parse() Line 43
 	EAR Binaural Monitoring.vst3!adm::getCommonDefinitions() Line 163	
 	EAR Binaural Monitoring.vst3!adm::parseXml(std::basic_istream<char,std::char_traits<char>> & stream, adm::xml::ParserOptions options) Line 18
 	EAR Binaural Monitoring.vst3!AdmPresetDefinitionsHelper::AdmPresetDefinitionsHelper() Line 54
 	EAR Binaural Monitoring.vst3!AdmPresetDefinitionsHelper::getSingleton() Line 65
 	EAR Binaural Monitoring.vst3!ear::plugin::BinauralMonitoringBackend::onSceneReceived(ear::plugin::proto::SceneStore store) Line 228
 	[External Code]	
	EAR Binaural Monitoring.vst3!ear::plugin::communication::MonitoringMetadataReceiver::handleReceive(std::error_code ec, nng::Message message) Line 65
 	[External Code]	
 	EAR Binaural Monitoring.vst3!nng::detail::AsyncReadAction::operator()() Line 261
 	[External Code]	
 	EAR Binaural Monitoring.vst3!nng::AsyncIO::callback() Line 227

The attached project crashes reliably on windows - tested on 2 machines. REAPER v7 (and i think the other machine was v6.something). IIRC, the stack is bigger on MacOS so probably won't crash. bin mon crash project.zip

I see two solutions.

  1. Bump the threads stack size. We've done this elsewhere by calling the message processing it in yet another thread so we're not using NNGs thread. SceneGainsCalculator for normal LS monitoring plugins -
    // Called by NNG callback on thread with small stack.
    // Launch task in another thread to overcome stack limitation.
  2. Always instantiate AdmPresetDefinitionsHelper on main thread on plugin startup.

Maybe it makes sense for all NNG message callbacks to launch in their own thread? This is bound to trip us up again.

Tagging @rsjbailey for suggestions.

@firthm01 firthm01 added the bug Something isn't working label May 4, 2024
@firthm01
Copy link
Collaborator Author

MonitoringMetadataReceiver::handleReceive is the last common point between the Binaural and LS monitoring plugins branch off. Perhaps this is where we launch the thread and remove it from SceneGainsCalculator?

firthm01 added a commit that referenced this issue May 15, 2024
Fix was to launch message handlers in new threads from the common `MonitoringMetadataReceiver::handleReceive` method for both Bin and LS mon plugin message handlers. Message handlers modified to use const refs to `SceneStore` to prevent accidental copies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant