Fix #277 - Stack overflow in Bin Mon message handler

[firthm01]

As per #277, a crash would occur (probably only on windows) if the AdmPresetDefinitionsHandler was first instantiated as a singleton via an NNG message, due to the NNG threads having a small stack. We had previously overcome a similar issue in the LS monitoring plugins by wrapping SceneGainsCalculator::update code in a thread. I used a similar fix here, but it made sense for the thread launching to occur further up the call stack since for both the LS mons and Bin mon, the message handler call stems from MonitoringMetadataReceiver::handleReceive

LS mon call stack from message received to SceneGainsCalculator::update where a new thread would be launched:

	EAR Monitoring 9+10+3.vst3!ear::plugin::SceneGainsCalculator::update(ear::plugin::proto::SceneStore store) Line 56
 	EAR Monitoring 9+10+3.vst3!ear::plugin::MonitoringBackend::updateActiveGains(ear::plugin::proto::SceneStore store) Line 63
 	EAR Monitoring 9+10+3.vst3!ear::plugin::MonitoringBackend::onSceneReceived(ear::plugin::proto::SceneStore store) Line 52
 	[External Code]	
 	EAR Monitoring 9+10+3.vst3!ear::plugin::communication::MonitoringMetadataReceiver::handleReceive::__l9::<lambda_1>::operator()() Line 70
 	[External Code]	
 	EAR Binaural Monitoring.vst3!nng::detail::AsyncReadAction::operator()() Line 261
 	[External Code]	
 	EAR Binaural Monitoring.vst3!nng::AsyncIO::callback() Line 227

Bin mon call stack from message received to stack overflow (still on NNG thread)

	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

Fix was to launch message handlers in new threads from the common MonitoringMetadataReceiver::handleReceive method.

Closes #277

[firthm01]

This bit isn't ideal. I think this causes a copy. Using std::move makes the compiler complain because sceneStore is apparently a const here... not sure if that happens as part of the capture list?
I think as future.get() is blocking, we are safe in terms of lifetime.

[firthm01]

I believe this is fixed by d216065

[rsjbailey]

Would marking the lambda as mutable fix the const warning? I can't get a small example that looks the same to emit that warning so I'm guessing a bit. I should probably check out the project and try it properly.

[firthm01]

Just to clarify as the later commit makes my message confusing. This code causes a compiler warning;

auto future = std::async(std::launch::async, [this, sceneStore]() {
    handler_(std::move(sceneStore));
});

The current code (below) it's happy with, but I'm not sure if this still causes a copy from the capture list to passing in to handle_ ;

auto future = std::async(std::launch::async, [this, sceneStore = std::move(sceneStore)]() {
    handler_(sceneStore);
});

[rsjbailey]

Yeah, I think the mutable thing fixes it, so

auto future = std::async(std::launch::async, [this, sceneStore = std::move(sceneStore)]() mutable {
  handler_(std::move(sceneStore));
}

I'm a bit rusty, but I think under the hood a lambda creates a function object with the captures as data members, and by default the call operator is const so that repeated calls to the same lambda with the same arguments produce the same results, so it's as if you did

struct MyLambda {
  void operator()() const {
    that->handler(std::move(sceneStore)); // won't compile as sceneStore is const on a const object
  }
  MonitoringMetadataReceiver* that;
  proto::SceneStore sceneStore;
};

MonitoringMetadataReceiver::handleReceive() {
// snip ...
  MyLambda const lambda{this, std::move(sceneStore)};
  auto future = std::async(std::launch::async, lambda);
}

The mutable keyword just removes the consts

Bit weird, but if anything, the rest of the language is wrong and lambdas have the correct defaults.

[firthm01]

Ahh thanks for the explanation - that makes sense why it complains!

[rsjbailey]

I'm not sure why the handler doesn't just take a const reference, maybe it should? (I've not checked to see what it's doing at the other end, if it needs to keep hold of a copy then passing by value makes sense)

[firthm01]

Two handlers... both just read, so I think that makes sense just to pass a const ref.

Bin plugin does it all in https://github.com/ebu/ear-production-suite/blob/main/ear-production-suite-plugins/lib/src/binaural_monitoring_backend.cpp#L203

LS plugins a bit more complicated as it gets passed through a few funcs, but non of them need a non-const:
https://github.com/ebu/ear-production-suite/blob/main/ear-production-suite-plugins/lib/src/monitoring_backend.cpp#L50
https://github.com/ebu/ear-production-suite/blob/main/ear-production-suite-plugins/lib/src/monitoring_backend.cpp#L60
https://github.com/ebu/ear-production-suite/blob/main/ear-production-suite-plugins/lib/src/scene_gains_calculator.cpp#L56

[firthm01]

Changed to const refs in ab7adb9

[rsjbailey]

Looks good, could capture by reference in the async lambda too (as we're immediately blocking on it's completion)

auto future = std::async(std::launch::async, [this, &sceneStore]() {
    handler_(sceneStore);
});
future.get(); //blocking