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

[WIP] Split CheckRelease in receiving A-RELEASE req and sending A-RELEASE confirmation #70

Closed
wants to merge 2 commits into from

Conversation

fedjo
Copy link

@fedjo fedjo commented Jul 12, 2017

Current implementation of CheckRelease() either checks and releases the current association. As a result the usage of callback on_association_released is more or less useless because the association is already released.

Supposing that on this callback someone could implement a procedure that could fail (eg writing to a file) the user should have the opportunity to abort the association instead of sending a release confirmation primitive.

This PR lets the user abort an association within the on_association_released callback.

@coveralls
Copy link

coveralls commented Jul 12, 2017

Coverage Status

Changes Unknown when pulling e2ea83c on fedjo:release_assoc into ** on pydicom:master**.

@fedjo fedjo changed the title Split CheckRelease in receiving A-RELEASE req and sending A-RELEASE confirmation [WIP] Split CheckRelease in receiving A-RELEASE req and sending A-RELEASE confirmation Jul 14, 2017
@fedjo
Copy link
Author

fedjo commented Aug 28, 2017

any updates @scaramallion

existing tests cover the changes. So coverage is the same

@scaramallion
Copy link
Member

CheckRelease returns the release confirmation when a A-RELEASE request is received, so I don't think it should be split. If the user wants to send a release request they should use assoc_release

@fedjo
Copy link
Author

fedjo commented Sep 25, 2017

@scaramallion
CheckRelease to my opinion should not return the release confirmation when A-RELEASE request is received. This is what I understand at least from the name of the method. CheckRelease should only check whether an A-RELEASE request is received.
A separate Release should then be used to send the confirmation back.

The reason for this small change is because someone can execute crucial operations on the on_association_release method and if these operation fail then he/she has the opportunity not to send a release confirmation response.

For example have a look on this small code snippet:

 # Check for release request
 if self.acse.CheckRelease():
     # Callback trigger
     self.debug_association_released()
     try:
         self.ae.on_association_released()  # Here crucial operations could be executed
     except:
         self.abort()
     else:
         self.acse.Release()
     finally:
         self.kill()

@scaramallion
Copy link
Member

Implemented in ACSE refactor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants