Mercurial > hg > orthanc-stone
changeset 977:262a0244e9b2 toa2019090201
Added missing Unregister for objects that register by the broker + logs + guard in FetchContext
| author | Benjamin Golinvaux <bgo@osimis.io> |
|---|---|
| date | Mon, 02 Sep 2019 17:29:26 +0200 |
| parents | 3abc47e051c8 |
| children | 0e21ecafcc23 |
| files | Framework/Loaders/DicomStructureSetLoader.cpp Framework/Loaders/LoaderStateMachine.cpp Framework/Loaders/LoaderStateMachine.h Framework/Loaders/OrthancSeriesVolumeProgressiveLoader.cpp Framework/Loaders/OrthancSeriesVolumeProgressiveLoader.h Framework/Messages/ICallable.h Framework/Messages/IObservable.cpp Framework/Messages/IObserver.h Framework/Oracle/WebAssemblyOracle.cpp Framework/Scene2DViewport/MeasureTool.cpp |
| diffstat | 10 files changed, 107 insertions(+), 37 deletions(-) [+] |
line wrap: on
line diff
--- a/Framework/Loaders/DicomStructureSetLoader.cpp Sat Aug 31 13:50:11 2019 +0200 +++ b/Framework/Loaders/DicomStructureSetLoader.cpp Mon Sep 02 17:29:26 2019 +0200 @@ -204,10 +204,6 @@ it = instances.begin(); it != instances.end(); ++it) { std::auto_ptr<OrthancRestApiCommand> command(new OrthancRestApiCommand); -#if 0 - if (logbgo115) - LOG(TRACE) << " DicomStructureSetLoader::LoadStructure::Handle() about to schedule /tools/lookup command with LookupInstance on result"; -#endif command->SetUri("/tools/lookup"); command->SetMethod(Orthanc::HttpMethod_Post); command->SetBody(*it); @@ -217,21 +213,10 @@ // They should NOT be required for POST requests //command->SetHttpHeader("pragma", "no-cache"); //command->SetHttpHeader("cache-control", "no-cache"); -#if 0 - std::string itStr(*it); - if(itStr == "1.3.12.2.1107.5.1.4.66930.30000018062412550879500002198") { - if (logbgo115) - LOG(ERROR) << "******** BOGUS LOOKUPS FROM NOW ON ***********"; - logbgo233 = true; - } -#endif + command->SetPayload(new LookupInstance(loader, *it)); //LOG(TRACE) << "About to schedule a /tools/lookup POST request. URI = " << command->GetUri() << " Body size = " << (*it).size() << " Body = " << (*it) << "\n"; Schedule(command.release()); -#if 0 - if (logbgo115) - LOG(TRACE) << " DicomStructureSetLoader::LoadStructure::Handle() request scheduled. Command will be LookupInstance and post body is " << *it; -#endif } } };
--- a/Framework/Loaders/LoaderStateMachine.cpp Sat Aug 31 13:50:11 2019 +0200 +++ b/Framework/Loaders/LoaderStateMachine.cpp Mon Sep 02 17:29:26 2019 +0200 @@ -136,10 +136,9 @@ template <typename T> void LoaderStateMachine::HandleSuccessMessage(const T& message) { - LOG(TRACE) << "LoaderStateMachine(" << std::hex << this << std::dec << ")::HandleSuccessMessage()"; + LOG(TRACE) << "LoaderStateMachine(" << std::hex << this << std::dec << ")::HandleSuccessMessage(). Receiver fingerprint = " << GetFingerprint(); if (activeCommands_ <= 0) { LOG(ERROR) << "LoaderStateMachine(" << std::hex << this << std::dec << ")::HandleSuccessMessage : activeCommands_ should be > 0 but is: " << activeCommands_; - LOG(ERROR) << "LoaderStateMachine(" << std::hex << this << std::dec << ") fingerprint = " << GetFingerprint(); } else { activeCommands_--; @@ -162,6 +161,7 @@ IObservable& oracleObservable) : IObserver(oracleObservable.GetBroker()), oracle_(oracle), + oracleObservable_(oracleObservable), active_(false), simultaneousDownloads_(4), activeCommands_(0) @@ -187,6 +187,7 @@ LoaderStateMachine::~LoaderStateMachine() { + oracleObservable_.Unregister(this); LOG(TRACE) << "LoaderStateMachine(" << std::hex << this << std::dec << ")::~LoaderStateMachine()"; Clear(); }
--- a/Framework/Loaders/LoaderStateMachine.h Sat Aug 31 13:50:11 2019 +0200 +++ b/Framework/Loaders/LoaderStateMachine.h Mon Sep 02 17:29:26 2019 +0200 @@ -95,6 +95,7 @@ typedef std::list<IOracleCommand*> PendingCommands; IOracle& oracle_; + IObservable& oracleObservable_; bool active_; unsigned int simultaneousDownloads_; PendingCommands pendingCommands_;
--- a/Framework/Loaders/OrthancSeriesVolumeProgressiveLoader.cpp Sat Aug 31 13:50:11 2019 +0200 +++ b/Framework/Loaders/OrthancSeriesVolumeProgressiveLoader.cpp Mon Sep 02 17:29:26 2019 +0200 @@ -424,6 +424,7 @@ IObserver(oracleObservable.GetBroker()), IObservable(oracleObservable.GetBroker()), oracle_(oracle), + oracleObservable_(oracleObservable), active_(false), simultaneousDownloads_(4), volume_(volume), @@ -445,6 +446,7 @@ OrthancSeriesVolumeProgressiveLoader::~OrthancSeriesVolumeProgressiveLoader() { + oracleObservable_.Unregister(this); LOG(TRACE) << "OrthancSeriesVolumeProgressiveLoader::~OrthancSeriesVolumeProgressiveLoader()"; }
--- a/Framework/Loaders/OrthancSeriesVolumeProgressiveLoader.h Sat Aug 31 13:50:11 2019 +0200 +++ b/Framework/Loaders/OrthancSeriesVolumeProgressiveLoader.h Mon Sep 02 17:29:26 2019 +0200 @@ -106,6 +106,7 @@ void LoadJpegSliceContent(const GetOrthancWebViewerJpegCommand::SuccessMessage& message); IOracle& oracle_; + IObservable& oracleObservable_; bool active_; unsigned int simultaneousDownloads_; SeriesGeometry seriesGeometry_;
--- a/Framework/Messages/ICallable.h Sat Aug 31 13:50:11 2019 +0200 +++ b/Framework/Messages/ICallable.h Mon Sep 02 17:29:26 2019 +0200 @@ -78,13 +78,21 @@ void ApplyInternal(const TMessage& message) { - if (observerFingerprint_ != observer_.GetFingerprint()) + std::string currentFingerprint(observer_.GetFingerprint()); + if (observerFingerprint_ != currentFingerprint) { - LOG(TRACE) << "The observer at address " << std::hex << &observer_ << std::dec << ") has a different fingerprint than the one recorded at callback registration time. This means that it is not the same object as the one recorded, even though their addresses are the same. Callback will NOT be sent!"; - LOG(TRACE) << " recorded fingerprint = " << observerFingerprint_ << " current fingerprint = " << observer_.GetFingerprint(); + LOG(TRACE) << "The observer at address " << + std::hex << &observer_ << std::dec << + ") has a different fingerprint than the one recorded at callback " << + "registration time. This means that it is not the same object as " << + "the one recorded, even though their addresses are the same. " << + "Callback will NOT be sent!"; + LOG(TRACE) << " recorded fingerprint = " << observerFingerprint_ << + " current fingerprint = " << currentFingerprint; } else { + LOG(TRACE) << "The recorded fingerprint is " << observerFingerprint_ << " and the current fingerprint is " << currentFingerprint << " -- callable will be called."; (observer_.*function_) (message); } }
--- a/Framework/Messages/IObservable.cpp Sat Aug 31 13:50:11 2019 +0200 +++ b/Framework/Messages/IObservable.cpp Mon Sep 02 17:29:26 2019 +0200 @@ -66,6 +66,8 @@ void IObservable::Unregister(IObserver *observer) { + LOG(TRACE) << "IObservable::Unregister for IObserver at addr: " + << std::hex << observer << std::dec; // delete all callables from this observer for (Callables::iterator itCallableSet = callables_.begin(); itCallableSet != callables_.end(); ++itCallableSet) @@ -75,6 +77,8 @@ { if ((*itCallable)->GetObserver() == observer) { + LOG(TRACE) << " ** IObservable::Unregister : deleting callable: " + << std::hex << (*itCallable) << std::dec; delete *itCallable; itCallableSet->second.erase(itCallable++); } @@ -87,6 +91,8 @@ void IObservable::EmitMessageInternal(const IObserver* receiver, const IMessage& message) { + LOG(TRACE) << "IObservable::EmitMessageInternal receiver = " + << std::hex << receiver << std::dec; Callables::const_iterator found = callables_.find(message.GetIdentifier()); if (found != callables_.end()) @@ -119,9 +125,10 @@ void IObservable::EmitMessage(const IObserver& observer, const IMessage& message) { + LOG(TRACE) << "IObservable::EmitMessage observer = " + << std::hex << &observer << std::dec; EmitMessageInternal(&observer, message); } - void IObservable::RegisterForwarder(IMessageForwarder* forwarder) {
--- a/Framework/Messages/IObserver.h Sat Aug 31 13:50:11 2019 +0200 +++ b/Framework/Messages/IObserver.h Mon Sep 02 17:29:26 2019 +0200 @@ -54,8 +54,33 @@ virtual ~IObserver() { - LOG(TRACE) << "IObserver(" << std::hex << this << std::dec << ")::~IObserver : fingerprint_ == " << fingerprint_; - broker_.Unregister(*this); + try + { + LOG(TRACE) << "IObserver(" << std::hex << this << std::dec << ")::~IObserver : fingerprint_ == " << fingerprint_; + const char* deadMarker = "deadbeef-dead-dead-0000-0000deadbeef"; + ORTHANC_ASSERT(strlen(deadMarker) == 36); + memcpy(fingerprint_, deadMarker, 37); + broker_.Unregister(*this); + } + catch (const Orthanc::OrthancException& e) + { + if (e.HasDetails()) + { + LOG(ERROR) << "OrthancException in ~IObserver: " << e.What() << " Details: " << e.GetDetails(); + } + else + { + LOG(ERROR) << "OrthancException in ~IObserver: " << e.What(); + } + } + catch (const std::exception& e) + { + LOG(ERROR) << "std::exception in ~IObserver: " << e.what(); + } + catch (...) + { + LOG(ERROR) << "Unknown exception in ~IObserver"; + } } const char* GetFingerprint() const
--- a/Framework/Oracle/WebAssemblyOracle.cpp Sat Aug 31 13:50:11 2019 +0200 +++ b/Framework/Oracle/WebAssemblyOracle.cpp Mon Sep 02 17:29:26 2019 +0200 @@ -89,11 +89,21 @@ virtual void EmitMessage(const IObserver& receiver, const IMessage& message) { + LOG(TRACE) << "WebAssemblyOracle::Emitter::EmitMessage receiver = " + << std::hex << &receiver << std::dec; oracle_.EmitMessage(receiver, message); } }; + /** + This object is created on the heap for every http request. + It is deleted in the success (or error) callbacks. + This object references the receiver of the request. Since this is a raw + reference, we need additional checks to make sure we send the response to + the same object, for the object can be deleted and a new one recreated at the + same address (it often happens in the [single-threaded] browser context). + */ class WebAssemblyOracle::FetchContext : public boost::noncopyable { private: @@ -101,6 +111,7 @@ const IObserver& receiver_; std::auto_ptr<IOracleCommand> command_; std::string expectedContentType_; + std::string receiverFingerprint_; public: FetchContext(WebAssemblyOracle& oracle, @@ -110,8 +121,13 @@ emitter_(oracle), receiver_(receiver), command_(command), - expectedContentType_(expectedContentType) + expectedContentType_(expectedContentType), + receiverFingerprint_(receiver.GetFingerprint()) { + LOG(TRACE) << "WebAssemblyOracle::FetchContext::FetchContext() | " + << "receiver address = " << std::hex << &receiver << std::dec + << " with fingerprint = " << receiverFingerprint_; + if (command == NULL) { throw Orthanc::OrthancException(Orthanc::ErrorCode_NullPointer); @@ -125,6 +141,8 @@ void EmitMessage(const IMessage& message) { + LOG(TRACE) << "WebAssemblyOracle::FetchContext::EmitMessage receiver_ = " + << std::hex << &receiver_ << std::dec; emitter_.EmitMessage(receiver_, message); } @@ -198,15 +216,38 @@ // an UUID is 36 chars : 32 hex chars + 4 hyphens: char #0 --> char #35 // char #36 is \0. + bool callHandler = true; // TODO: remove this line because we are NOT allowed to call methods on GetReceiver that is maybe a dangling ref - if (context->GetReceiver().DoesFingerprintLookGood()) { - LOG(TRACE) << "SuccessCallback for object at address (" << std::hex << &(context->GetReceiver()) << std::dec << " with current fingerprint = " << context->GetReceiver().GetFingerprint(); + if (context->GetReceiver().DoesFingerprintLookGood()) + { + callHandler = true; + std::string currentFingerprint(context->GetReceiver().GetFingerprint()); + + LOG(TRACE) << "SuccessCallback for object at address (" << std::hex + << &(context->GetReceiver()) << std::dec + << " with current fingerprint = " << currentFingerprint + << ". Fingerprint looks OK"; + + if (currentFingerprint != context->receiverFingerprint_) + { + LOG(TRACE) << " ** SuccessCallback: BUT currentFingerprint != " + << "receiverFingerprint_(" << context->receiverFingerprint_ << ")"; + callHandler = false; + } + else + { + LOG(TRACE) << " ** SuccessCallback: FetchContext-level " + << "fingerprints are the same: " + << context->receiverFingerprint_ + << " ---> oracle will dispatch the response to observer: " + << std::hex << &(context->GetReceiver()) << std::dec; + } } else { LOG(TRACE) << "SuccessCallback for object at address (" << std::hex << &(context->GetReceiver()) << std::dec << " with current fingerprint is XXXXX -- NOT A VALID FINGERPRINT! OBJECT IS READ ! CALLBACK WILL NOT BE CALLED!"; + callHandler = false; } - if (fetch->userData == NULL) { @@ -261,11 +302,13 @@ } else { - if (context->GetReceiver().DoesFingerprintLookGood()) { + if (callHandler) + { switch (context->GetCommand().GetType()) { case IOracleCommand::Type_OrthancRestApi: { + LOG(TRACE) << "WebAssemblyOracle::FetchContext::SuccessCallback. About to call context->EmitMessage(message);"; OrthancRestApiCommand::SuccessMessage message (context->GetTypedCommand<OrthancRestApiCommand>(), headers, answer); context->EmitMessage(message); @@ -611,13 +654,10 @@ void WebAssemblyOracle::Schedule(const IObserver& receiver, IOracleCommand* command) { -#if 0 - if (logbgo233) { - if (logbgo115) - LOG(TRACE) << " WebAssemblyOracle::Schedule command addr " << - std::hex << command << std::dec; - } -#endif + LOG(TRACE) << "WebAssemblyOracle::Schedule : receiver = " + << std::hex << &receiver << std::dec + << " | Current fingerprint is " << receiver.GetFingerprint(); + std::auto_ptr<IOracleCommand> protection(command); if (command == NULL)