pydicom / pynetdicom

A Python implementation of the DICOM networking protocol
https://pydicom.github.io/pynetdicom
MIT License
500 stars 176 forks source link

Possible memory leak when manually aborting an association #912

Open pchristos opened 7 months ago

pchristos commented 7 months ago

Hi,

I have investigated a memory leak, which seems to be related to attempts of manually aborting associations.

Minimal example:

def _on_pdu_recv(self, event):
    # Method bound to `EVT_PDU_RECV` event.
    if isinstance(event.pdu, A_RELEASE_RQ):
        self._on_handle_a_release_rq(event)

def _on_handle_a_release_rq(self, event):
    try:
        do_something_with_event(event)
    except Exception:
        # The following line of code kills the Association thread. The DUL's 
        # FSM is stuck in `Sta6`, thus it is not killed by the Association 
        # thread's `kill()` method, which is invoked by the calling to `abort()` 
        # below. As a result, the `kill()` method is stuck in a `while` loop
        # trying to stop the DUL.
        event.assoc.abort()
        # Due to the above, the line below is never executed.
        do_something_else()

The above seems like a straight-forward way of aborting, but leads to leaking of DUL threads.

I have found that the below alternative works better:

def _on_pdu_recv(self, event):
    # Method bound to `EVT_PDU_RECV` event.
    if isinstance(event.pdu, A_RELEASE_RQ):
        self._on_handle_a_release_rq(event)

def _on_handle_a_release_rq(self, event):
    try:
        do_something_with_event(event)
    except Exception:
        # Manually create the PDU to abort.
        abort_pdu = A_ABORT_RQ()
        abort_pdu.source = 0x02
        abort_pdu.reason_diagnostic = 0x00
        event.assoc.dul.socket.send(abort_pdu.encode())
        event.assoc.dul.assoc.is_aborted = True
        event.assoc.dul.assoc.is_established = False
        # Hard shutdown of the Association and DUL reactors.
        event.assoc.dul.assoc._kill = True
        event.assoc.dul._kill_thread = True
        # The line below is called.
        do_something_else()

The above approach is based on this.

I am wondering what is actually the best approach to abort an association at any time. Thoughts?

Originally posted by @pchristos in https://github.com/pydicom/pynetdicom/issues/652#issuecomment-1387319121

pydicom 2.3.1 pynetdicom 2.0.2

scaramallion commented 7 months ago

Yeah, there's some iffy behaviour in shutting the DUL down properly that will be fixed in the next release.

pchristos commented 6 months ago

Thanks! ETA for the next release?

scaramallion commented 6 months ago

It'll be quite a while, I think. I've been focussing on pydicom's v3.0 release.

pchristos commented 6 months ago

In that case, what's your recommended approach? Is the proposed alternative a valid approach for now?

pchristos commented 2 months ago

Hey, any updates on this?

scaramallion commented 2 months ago

Nothing so far, but probably soonish.