# HG changeset patch # User Sebastien Jodogne # Date 1603711416 -3600 # Node ID 6f5d4bfb2c901de0435a95ab868479a99daa2cfb # Parent c046d559edb340a5af4b4f2b2995d09799acdab8 C-GET SCP: Fix responses and handling of cancel diff -r c046d559edb3 -r 6f5d4bfb2c90 NEWS --- a/NEWS Tue Oct 20 09:52:42 2020 +0200 +++ b/NEWS Mon Oct 26 12:23:36 2020 +0100 @@ -1,7 +1,7 @@ Pending changes in the mainline =============================== -* Fix reporting of client-side store warnings/errors in C-GET SCP +* C-GET SCP: Fix responses and handling of cancel Version 1.8.0 (2020-10-16) diff -r c046d559edb3 -r 6f5d4bfb2c90 OrthancFramework/Sources/DicomNetworking/IGetRequestHandler.h --- a/OrthancFramework/Sources/DicomNetworking/IGetRequestHandler.h Tue Oct 20 09:52:42 2020 +0200 +++ b/OrthancFramework/Sources/DicomNetworking/IGetRequestHandler.h Mon Oct 26 12:23:36 2020 +0100 @@ -34,13 +34,6 @@ class IGetRequestHandler : boost::noncopyable { public: - enum Status - { - Status_Success, - Status_Failure, - Status_Warning - }; - virtual ~IGetRequestHandler() { } @@ -52,10 +45,9 @@ uint32_t timeout) = 0; virtual unsigned int GetSubOperationCount() const = 0; - - virtual Status DoNext(T_ASC_Association *) = 0; - - virtual unsigned int GetRemainingCount() const = 0; + + // Must return "false" iff. a "Cancel" was returned by the remote SCU + virtual bool DoNext(T_ASC_Association *) = 0; virtual unsigned int GetCompletedCount() const = 0; diff -r c046d559edb3 -r 6f5d4bfb2c90 OrthancFramework/Sources/DicomNetworking/Internals/GetScp.cpp --- a/OrthancFramework/Sources/DicomNetworking/Internals/GetScp.cpp Tue Oct 20 09:52:42 2020 +0200 +++ b/OrthancFramework/Sources/DicomNetworking/Internals/GetScp.cpp Mon Oct 26 12:23:36 2020 +0100 @@ -99,12 +99,14 @@ std::string remoteAet_; std::string calledAet_; int timeout_; + bool canceled_; GetScpData() : handler_(NULL), lastRequest_(NULL), assoc_(NULL), - timeout_(0) + timeout_(0), + canceled_(false) { }; }; @@ -129,6 +131,56 @@ } } + + static void FillResponse(T_DIMSE_C_GetRSP& response, + DcmDataset** failedIdentifiers, + const IGetRequestHandler& handler) + { + response.DimseStatus = STATUS_Success; + + size_t processedCount = (handler.GetCompletedCount() + + handler.GetFailedCount() + + handler.GetWarningCount()); + + if (processedCount > handler.GetSubOperationCount()) + { + throw OrthancException(ErrorCode_InternalError); + } + + response.NumberOfRemainingSubOperations = (handler.GetSubOperationCount() - processedCount); + response.NumberOfCompletedSubOperations = handler.GetCompletedCount(); + response.NumberOfFailedSubOperations = handler.GetFailedCount(); + response.NumberOfWarningSubOperations = handler.GetWarningCount(); + + // http://dicom.nema.org/medical/dicom/current/output/chtml/part04/sect_C.4.3.3.html + + if (handler.GetFailedCount() > 0 || + handler.GetWarningCount() > 0) + { + /** + * "Warning if one or more sub-operations were successfully + * completed and one or more sub-operations were unsuccessful + * or had a status of warning. Warning if all sub-operations + * had a status of Warning" + **/ + response.DimseStatus = STATUS_GET_Warning_SubOperationsCompleteOneOrMoreFailures; + } + + if (handler.GetFailedCount() > 0 && + handler.GetFailedCount() == handler.GetSubOperationCount()) + { + /** + * "Failure or Refused if all sub-operations were + * unsuccessful." => We choose to generate a "Refused - Out + * of Resources - Unable to perform suboperations" status. + */ + response.DimseStatus = STATUS_GET_Refused_OutOfResourcesSubOperations; + } + + *failedIdentifiers = BuildFailedInstanceList(handler.GetFailedUids()); + } + + static void GetScpCallback( /* in */ void *callbackData, @@ -141,6 +193,9 @@ DcmDataset **responseIdentifiers, DcmDataset **statusDetail) { + assert(response != NULL); + assert(responseIdentifiers != NULL); + bzero(response, sizeof(T_DIMSE_C_GetRSP)); *statusDetail = NULL; *responseIdentifiers = NULL; @@ -180,68 +235,52 @@ return; } - if (data.handler_->GetRemainingCount() == 0) + if (data.canceled_) { - response->DimseStatus = STATUS_Success; + LOG(ERROR) << "IGetRequestHandler Failed: Cannot pursue a request that was canceled by the SCU"; + response->DimseStatus = STATUS_GET_Failed_UnableToProcess; + return; + } + + if (data.handler_->GetSubOperationCount() == + data.handler_->GetCompletedCount() + + data.handler_->GetFailedCount() + + data.handler_->GetWarningCount()) + { + // We're all done + FillResponse(*response, responseIdentifiers, *data.handler_); } else { - IGetRequestHandler::Status status; - + bool isContinue; + try { - status = data.handler_->DoNext(data.assoc_); + isContinue = data.handler_->DoNext(data.assoc_); } catch (OrthancException& e) { // Internal error! LOG(ERROR) << "IGetRequestHandler Failed: " << e.What(); + FillResponse(*response, responseIdentifiers, *data.handler_); + + // Fix the status code that is computed by "FillResponse()" response->DimseStatus = STATUS_GET_Failed_UnableToProcess; return; } - - if (status == STATUS_Success) + + FillResponse(*response, responseIdentifiers, *data.handler_); + + if (isContinue) { - if (responseCount < static_cast(data.handler_->GetRemainingCount())) - { - response->DimseStatus = STATUS_Pending; - } - else - { - response->DimseStatus = STATUS_Success; - } + response->DimseStatus = STATUS_Pending; } else { - response->DimseStatus = STATUS_GET_Failed_UnableToProcess; - - if (data.handler_->GetFailedCount() > 0 || - data.handler_->GetWarningCount() > 0) - { - response->DimseStatus = STATUS_GET_Warning_SubOperationsCompleteOneOrMoreFailures; - } - - /* - * if all the sub-operations failed then we need to generate - * a failed or refused status. cf. DICOM part 4, C.4.3.3.1 - * we choose to generate a "Refused - Out of Resources - - * Unable to perform suboperations" status. - */ - if ((data.handler_->GetFailedCount() > 0) && - ((data.handler_->GetCompletedCount() + - data.handler_->GetWarningCount()) == 0)) - { - response->DimseStatus = STATUS_GET_Refused_OutOfResourcesSubOperations; - } - - *responseIdentifiers = BuildFailedInstanceList(data.handler_->GetFailedUids()); + response->DimseStatus = STATUS_GET_Cancel_SubOperationsTerminatedDueToCancelIndication; + data.canceled_ = true; } } - - response->NumberOfRemainingSubOperations = data.handler_->GetRemainingCount(); - response->NumberOfCompletedSubOperations = data.handler_->GetCompletedCount(); - response->NumberOfFailedSubOperations = data.handler_->GetFailedCount(); - response->NumberOfWarningSubOperations = data.handler_->GetWarningCount(); } } diff -r c046d559edb3 -r 6f5d4bfb2c90 OrthancServer/Sources/OrthancGetRequestHandler.cpp --- a/OrthancServer/Sources/OrthancGetRequestHandler.cpp Tue Oct 20 09:52:42 2020 +0200 +++ b/OrthancServer/Sources/OrthancGetRequestHandler.cpp Mon Oct 26 12:23:36 2020 +0100 @@ -66,12 +66,12 @@ } } - OrthancGetRequestHandler::Status - OrthancGetRequestHandler::DoNext(T_ASC_Association* assoc) + + bool OrthancGetRequestHandler::DoNext(T_ASC_Association* assoc) { if (position_ >= instances_.size()) { - return Status_Failure; + throw OrthancException(ErrorCode_ParameterOutOfRange); } const std::string& id = instances_[position_++]; @@ -81,7 +81,7 @@ if (dicom.empty()) { - return Status_Failure; + throw OrthancException(ErrorCode_BadFileFormat); } std::unique_ptr parsed( @@ -107,17 +107,7 @@ std::string sopClassUid(a.c_str()); std::string sopInstanceUid(b.c_str()); - Status status = PerformGetSubOp(assoc, sopClassUid, sopInstanceUid, parsed.release()); - - if (getCancelled_) - { - LOG(INFO) << "C-GET SCP: Received C-Cancel RQ"; - return Status_Failure; - } - else - { - return status; - } + return PerformGetSubOp(assoc, sopClassUid, sopInstanceUid, parsed.release()); } @@ -231,11 +221,10 @@ } - OrthancGetRequestHandler::Status - OrthancGetRequestHandler::PerformGetSubOp(T_ASC_Association* assoc, - const std::string& sopClassUid, - const std::string& sopInstanceUid, - DcmFileFormat* dicomRaw) + bool OrthancGetRequestHandler::PerformGetSubOp(T_ASC_Association* assoc, + const std::string& sopClassUid, + const std::string& sopInstanceUid, + DcmFileFormat* dicomRaw) { assert(dicomRaw != NULL); std::unique_ptr dicom(dicomRaw); @@ -243,11 +232,12 @@ DicomTransferSyntax sourceSyntax; if (!FromDcmtkBridge::LookupOrthancTransferSyntax(sourceSyntax, *dicom)) { - nFailed_++; + failedCount_++; AddFailedUIDInstance(sopInstanceUid); - LOG(ERROR) << "C-GET SCP: Unknown transfer syntax: (" - << dcmSOPClassUIDToModality(sopClassUid.c_str(), "OT") << ") " << sopClassUid; - return Status_Failure; + throw OrthancException(ErrorCode_NetworkProtocol, + "C-GET SCP: Unknown transfer syntax: (" + + std::string(dcmSOPClassUIDToModality(sopClassUid.c_str(), "OT")) + + ") " + sopClassUid); } bool allowTranscoding = (context_.IsTranscodeDicomProtocol() && @@ -259,11 +249,12 @@ sourceSyntax, allowTranscoding) || presId == 0) { - nFailed_++; + failedCount_++; AddFailedUIDInstance(sopInstanceUid); - LOG(ERROR) << "C-GET SCP: storeSCU: No presentation context for: (" - << dcmSOPClassUIDToModality(sopClassUid.c_str(), "OT") << ") " << sopClassUid; - return Status_Failure; + throw OrthancException(ErrorCode_NetworkProtocol, + "C-GET SCP: storeSCU: No presentation context for: (" + + std::string(dcmSOPClassUIDToModality(sopClassUid.c_str(), "OT")) + + ") " + sopClassUid); } else { @@ -281,11 +272,12 @@ pc.acceptedRole != ASC_SC_ROLE_SCUSCP) { // the role is not appropriate - nFailed_++; + failedCount_++; AddFailedUIDInstance(sopInstanceUid); - LOG(ERROR) << "C-GET SCP: storeSCU: [No presentation context with requestor SCP role for: (" - << dcmSOPClassUIDToModality(sopClassUid.c_str(), "OT") << ") " << sopClassUid; - return Status_Failure; + throw OrthancException(ErrorCode_NetworkProtocol, + "C-GET SCP: storeSCU: [No presentation context with requestor SCP role for: (" + + std::string(dcmSOPClassUIDToModality(sopClassUid.c_str(), "OT")) + + ") " + sopClassUid); } } @@ -348,37 +340,29 @@ else { // Cannot transcode - nFailed_++; + failedCount_++; AddFailedUIDInstance(sopInstanceUid); - LOG(ERROR) << "C-GET SCP: Cannot transcode " << sopClassUid - << " from transfer syntax " << GetTransferSyntaxUid(sourceSyntax) - << " to " << GetTransferSyntaxUid(selectedSyntax); - return Status_Failure; + throw OrthancException(ErrorCode_NotImplemented, + "C-GET SCP: Cannot transcode " + sopClassUid + + " from transfer syntax " + GetTransferSyntaxUid(sourceSyntax) + + " to " + GetTransferSyntaxUid(selectedSyntax)); } } - Status status; + bool isContinue; + if (cond.good()) { if (cancelParameters.cancelEncountered) { - if (origPresId_ == cancelParameters.presId && - origMsgId_ == cancelParameters.req.MessageIDBeingRespondedTo) - { - getCancelled_ = true; - } - else - { - LOG(ERROR) << "C-GET SCP: Unexpected C-Cancel-RQ encountered: pid=" << (int)cancelParameters.presId - << ", mid=" << (int)cancelParameters.req.MessageIDBeingRespondedTo; - } + LOG(INFO) << "C-GET SCP: Received C-Cancel RQ"; + isContinue = false; } - - if (rsp.DimseStatus == STATUS_Success) + else if (rsp.DimseStatus == STATUS_Success) { // everything ok - nCompleted_++; - status = Status_Success; + completedCount_++; + isContinue = true; } else if ((rsp.DimseStatus & 0xf000) == 0xb000) { @@ -386,25 +370,25 @@ warningCount_++; LOG(ERROR) << "C-GET SCP: Store Warning: Response Status: " << DU_cstoreStatusString(rsp.DimseStatus); - status = Status_Warning; + isContinue = true; } else { - nFailed_++; + failedCount_++; AddFailedUIDInstance(sopInstanceUid); // print a status message LOG(ERROR) << "C-GET SCP: Store Failed: Response Status: " << DU_cstoreStatusString(rsp.DimseStatus); - status = Status_Failure; + isContinue = true; } } else { - nFailed_++; + failedCount_++; AddFailedUIDInstance(sopInstanceUid); OFString temp_str; LOG(ERROR) << "C-GET SCP: storeSCU: Store Request Failed: " << DimseCondition::dump(temp_str, cond); - status = Status_Failure; + isContinue = true; } if (stDetail.get() != NULL) @@ -418,7 +402,7 @@ LOG(INFO) << " Status Detail: " << s.str(); } - return status; + return isContinue; } bool OrthancGetRequestHandler::LookupIdentifiers(std::list& publicIds, @@ -492,14 +476,12 @@ OrthancGetRequestHandler::OrthancGetRequestHandler(ServerContext& context) : - context_(context), - getCancelled_(false) + context_(context) { position_ = 0; - nRemaining_ = 0; - nCompleted_ = 0; + completedCount_ = 0; warningCount_ = 0; - nFailed_ = 0; + failedCount_ = 0; timeout_ = 0; } @@ -575,11 +557,9 @@ } failedUIDs_.clear(); - getCancelled_ = false; - nRemaining_ = GetSubOperationCount(); - nCompleted_ = 0; - nFailed_ = 0; + completedCount_ = 0; + failedCount_ = 0; warningCount_ = 0; timeout_ = timeout; diff -r c046d559edb3 -r 6f5d4bfb2c90 OrthancServer/Sources/OrthancGetRequestHandler.h --- a/OrthancServer/Sources/OrthancGetRequestHandler.h Tue Oct 20 09:52:42 2020 +0200 +++ b/OrthancServer/Sources/OrthancGetRequestHandler.h Mon Oct 26 12:23:36 2020 +0100 @@ -56,26 +56,22 @@ RemoteModalityParameters remote_; std::string originatorAet_; - unsigned int nRemaining_; - unsigned int nCompleted_; + unsigned int completedCount_; unsigned int warningCount_; - unsigned int nFailed_; + unsigned int failedCount_; std::string failedUIDs_; - DIC_US origMsgId_; - T_ASC_PresentationContextID origPresId_; - - bool getCancelled_; uint32_t timeout_; bool LookupIdentifiers(std::list& publicIds, ResourceType level, const DicomMap& input) const; - Status PerformGetSubOp(T_ASC_Association *assoc, - const std::string& sopClassUid, - const std::string& sopInstanceUid, - DcmFileFormat* datasetRaw); + // Returns "false" iff cancel + bool PerformGetSubOp(T_ASC_Association *assoc, + const std::string& sopClassUid, + const std::string& sopInstanceUid, + DcmFileFormat* datasetRaw); void AddFailedUIDInstance(const std::string& sopInstance); @@ -87,22 +83,17 @@ const std::string& originatorAet, const std::string& calledAet, uint32_t timeout) ORTHANC_OVERRIDE; - - virtual Status DoNext(T_ASC_Association *assoc) ORTHANC_OVERRIDE; + + virtual bool DoNext(T_ASC_Association *assoc) ORTHANC_OVERRIDE; virtual unsigned int GetSubOperationCount() const ORTHANC_OVERRIDE { return static_cast(instances_.size()); } - virtual unsigned int GetRemainingCount() const ORTHANC_OVERRIDE - { - return nRemaining_; - } - virtual unsigned int GetCompletedCount() const ORTHANC_OVERRIDE { - return nCompleted_; + return completedCount_; } virtual unsigned int GetWarningCount() const ORTHANC_OVERRIDE @@ -112,7 +103,7 @@ virtual unsigned int GetFailedCount() const ORTHANC_OVERRIDE { - return nFailed_; + return failedCount_; } virtual const std::string& GetFailedUids() const ORTHANC_OVERRIDE