Repository: pydicom/pynetdicom

Issue:
Established associations not timing out
### Description
Association should end with abort once commands have been completed and timeout expires.

### Expected behaviour
At the end of a script that requested an association the association should abort unless already ended through `assoc.release()` or `assoc.abort()` when the timeout ends.

### Actual behaviour
A requested association stays established, timeouts don't appear to have any effect.  Seems to work OK with acceptors.

### Steps to reproduce
Request a new association, send a DIMSE-C service request and don't release it.

### Your environment
Current master

Discussion:

Patch:
diff --git a/docs/changelog/v1.2.0.rst b/docs/changelog/v1.2.0.rst
index a239bf92e..e7e28c5d4 100644
--- a/docs/changelog/v1.2.0.rst
+++ b/docs/changelog/v1.2.0.rst
@@ -5,6 +5,9 @@
 
 Fixes
 .....
+* Fixed `network_timeout` not working when acting as the association requestor
+  and an A-ABORT not being sent when the timeout expired. (:issue:`286`)
+* Fixed changing `network_timeout` value not propagating to the actual timer.
 
 Enhancements
 ............
diff --git a/pynetdicom/association.py b/pynetdicom/association.py
index caa4e786b..c2c5bcd69 100644
--- a/pynetdicom/association.py
+++ b/pynetdicom/association.py
@@ -126,11 +126,6 @@ class Association(threading.Thread):
         self._ae = ae
         self.mode = mode
 
-        # Timeouts (in seconds)
-        self.acse_timeout = self.ae.acse_timeout
-        self.dimse_timeout = self.ae.dimse_timeout
-        self.network_timeout = self.ae.network_timeout
-
         # Represents the association requestor and acceptor users
         self.requestor = ServiceUser(self, MODE_REQUESTOR)
         self.acceptor = ServiceUser(self, MODE_ACCEPTOR)
@@ -148,7 +143,12 @@ class Association(threading.Thread):
         # Service providers
         self.acse = ACSE()
         self.dul = DULServiceProvider(self)
-        self.dimse = DIMSEServiceProvider(self.dul, self.dimse_timeout)
+        self.dimse = DIMSEServiceProvider(self.dul, self.ae.dimse_timeout)
+
+        # Timeouts (in seconds), needs to be set after DUL init
+        self.acse_timeout = self.ae.acse_timeout
+        self.dimse_timeout = self.ae.dimse_timeout
+        self.network_timeout = self.ae.network_timeout
 
         # Kills the thread loop in run()
         self._kill = False
@@ -180,6 +180,28 @@ class Association(threading.Thread):
         """Return a list of accepted Presentation Contexts."""
         return self._accepted_cx
 
+    @property
+    def acse_timeout(self):
+        """Return the ACSE timeout in seconds."""
+        return self._acse_timeout
+
+    @acse_timeout.setter
+    def acse_timeout(self, value):
+        """Set the ACSE timeout using numeric or None."""
+        self.dul.artim_timer.timeout = value
+        self._acse_timeout = value
+
+    @property
+    def network_timeout(self):
+        """Return the network timeout in seconds."""
+        return self._network_timeout
+
+    @network_timeout.setter
+    def network_timeout(self, value):
+        """Set the network timeout using numeric or None."""
+        self.dul._idle_timer.timeout = value
+        self._network_timeout = value
+
     @property
     def ae(self):
         """Return the Association's parent ApplicationEntity."""
@@ -305,7 +327,7 @@ class Association(threading.Thread):
         """Kill the main association thread loop."""
         self._kill = True
         self.is_established = False
-        while not self.dul.stop_dul():
+        while self.dul.is_alive() and not self.dul.stop_dul():
             time.sleep(0.01)
 
     @property
@@ -539,8 +561,9 @@ class Association(threading.Thread):
 
             # Check if idle timer has expired
             if self.dul.idle_timer_expired():
-                LOGGER.debug("DUL timeout expired")
                 self.abort()
+                self.kill()
+                return
 
     def _run_as_requestor(self):
         """Run the association as the requestor."""
@@ -579,6 +602,7 @@ class Association(threading.Thread):
 
             # Check if idle timer has expired
             if self.dul.idle_timer_expired():
+                self.abort()
                 self.kill()
                 return
 
diff --git a/pynetdicom/dimse.py b/pynetdicom/dimse.py
index 66bf94036..7678976de 100644
--- a/pynetdicom/dimse.py
+++ b/pynetdicom/dimse.py
@@ -266,7 +266,7 @@ class DIMSEServiceProvider(object):
                 # Because we only progress once the next PDU arrives to be
                 #   peeked at, the DIMSE timeout in receive_pdu() doesn't
                 #   actually do anything.
-                if timeout.is_expired:
+                if timeout.expired:
                     return None, None
 
                 # Race condition: sometimes the DUL will be killed before the
diff --git a/pynetdicom/dul.py b/pynetdicom/dul.py
index e2831305e..f9c1c337e 100644
--- a/pynetdicom/dul.py
+++ b/pynetdicom/dul.py
@@ -76,8 +76,9 @@ class DULServiceProvider(Thread):
         self.to_user_queue = queue.Queue()
 
         # Set the (network) idle and ARTIM timers
-        self._idle_timer = Timer(self.assoc.network_timeout)
-        self.artim_timer = Timer(self.assoc.acse_timeout)
+        # Timeouts gets set after DUL init so these are temporary
+        self._idle_timer = Timer(60)
+        self.artim_timer = Timer(30)
 
         # State machine - PS3.8 Section 9.2
         self.state_machine = StateMachine(self)
@@ -142,15 +143,9 @@ class DULServiceProvider(Thread):
         Returns
         -------
         bool
-            True if the idle timer has expired, False otherwise
+            True if the idle timer has expired, False otherwise.
         """
-        if self._idle_timer is None:
-            return False
-
-        if self._idle_timer.is_expired:
-            return True
-
-        return False
+        return self._idle_timer.expired
 
     def _is_artim_expired(self):
         """Return if the state machine's ARTIM timer has expired.
@@ -162,7 +157,7 @@ class DULServiceProvider(Thread):
         bool
             True if the ARTIM timer has expired, False otherwise
         """
-        if self.artim_timer.is_expired:
+        if self.artim_timer.expired:
             self.event_queue.put('Evt18')
             return True
 
@@ -380,8 +375,7 @@ class DULServiceProvider(Thread):
         categorises it and add its to the `to_user_queue`.
         """
         # Main DUL loop
-        if self._idle_timer is not None:
-            self._idle_timer.start()
+        self._idle_timer.start()
 
         while True:
             # This effectively controls how often the DUL checks the network
@@ -392,8 +386,7 @@ class DULServiceProvider(Thread):
 
             # Check the connection for incoming data
             try:
-                if self._is_transport_event() and self._idle_timer is not None:
-                    self._idle_timer.timeout_seconds = self.network_timeout
+                if self._is_transport_event():
                     self._idle_timer.restart()
                 elif self._check_incoming_primitive():
                     pass
diff --git a/pynetdicom/fsm.py b/pynetdicom/fsm.py
index 5ae9c5972..c5eb2c6e0 100644
--- a/pynetdicom/fsm.py
+++ b/pynetdicom/fsm.py
@@ -268,7 +268,6 @@ def AE_5(dul):
         Sta2, the next state of the state machine
     """
     # Start ARTIM timer
-    dul.artim_timer.timeout_seconds = dul.assoc.acse_timeout
     dul.artim_timer.start()
 
     return 'Sta2'
@@ -325,7 +324,6 @@ def AE_6(dul):
         dul.assoc.acse.debug_send_associate_rj(dul.pdu)
 
         dul.socket.send(dul.pdu.encode())
-        dul.artim_timer.timeout_seconds = dul.assoc.acse_timeout
         dul.artim_timer.start()
 
         return 'Sta13'
@@ -400,8 +398,6 @@ def AE_8(dul):
     dul.assoc.acse.debug_send_associate_rj(dul.pdu)
 
     dul.socket.send(dul.pdu.encode())
-
-    dul.artim_timer.timeout_seconds = dul.assoc.acse_timeout
     dul.artim_timer.start()
 
     return 'Sta13'
@@ -589,7 +585,6 @@ def AR_4(dul):
     dul.assoc.acse.debug_send_release_rp(dul.pdu)
 
     dul.socket.send(dul.pdu.encode())
-    dul.artim_timer.timeout_seconds = dul.assoc.acse_timeout
     dul.artim_timer.start()
 
     return 'Sta13'
@@ -812,7 +807,6 @@ def AA_1(dul):
     dul.assoc.acse.debug_send_abort(dul.pdu)
 
     dul.socket.send(dul.pdu.encode())
-    dul.artim_timer.timeout_seconds = dul.assoc.acse_timeout
     dul.artim_timer.restart()
 
     return 'Sta13'
@@ -1047,7 +1041,6 @@ def AA_8(dul):
 
     # Issue A-P-ABORT to user
     dul.to_user_queue.put(dul.primitive)
-    dul.artim_timer.timeout_seconds = dul.assoc.acse_timeout
     dul.artim_timer.start()
 
     return 'Sta13'
diff --git a/pynetdicom/timer.py b/pynetdicom/timer.py
index d4ec1b619..316d29d55 100644
--- a/pynetdicom/timer.py
+++ b/pynetdicom/timer.py
@@ -4,6 +4,7 @@ A generic timer class suitable for use as the DICOM UL's ARTIM timer.
 import logging
 import time
 
+
 LOGGER = logging.getLogger('pynetdicom.artim')
 
 
@@ -14,41 +15,41 @@ class Timer(object):
     used by the state machine to monitor connection and response timeouts.
     This class may also be used as a general purpose expiry timer.
 
+    A `timeout` of None implies that `expired` always returns False and
+    `remaining` always returns 1.
+    A `timeout` of float implies that:
+
+    - If not yet started, `expired` returns False and remaining returns
+      `timeout`
+    - If started then `expired` returns False until the time since starting
+      is greater than `timeout` after which it returns True. `remaining`
+      returns the number of seconds until `expired` returns True (will
+      return negative value after expiry)
+    - If started then stopped before the timeout then `expired` returns
+      False, if stopped after the time since starting is greater than `timeout`
+      then returns True. `remaining` always returns the number of seconds
+      until `expired` returns True.
+
     References
     ----------
 
     * DICOM Standard, Part 8, Section 9.1.5.
     """
-    def __init__(self, max_number_seconds):
+    def __init__(self, timeout):
         """Create a new Timer.
 
         Parameters
         ---------
-        max_number_seconds : int or float or None
+        timeout : numeric or None
             The number of seconds before the timer expires. A value of None
-            means no timeout.
+            means the timer never expires.
         """
         self._start_time = None
-        self.timeout_seconds = max_number_seconds
-
-    def start(self):
-        """Resets and starts the timer running."""
-        self._start_time = time.time()
-
-    def stop(self):
-        """Stops the timer and resets it."""
-        self._start_time = None
-
-    def restart(self):
-        """Restart the timer.
-
-        If the timer has already started then stop it, reset it and start it.
-        If the timer isn't running then reset it and start it.
-        """
-        self.start()
+        self._end_time = None
+        self.timeout = timeout
 
     @property
-    def is_expired(self):
+    def expired(self):
         """Check if the timer has expired.
 
         Returns
@@ -56,41 +57,68 @@ class Timer(object):
         bool
             True if the timer has expired, False otherwise
         """
-        if self._start_time is not None and self.timeout_seconds is not None:
-            if self.time_remaining < 0:
-                return True
+        # Timer never expires
+        if self.timeout is None:
+            return False
+
+        # Timer hasn't started
+        if self._start_time is None:
+            return False
+
+        # Timer has started
+        if self.remaining < 0:
+            return True
 
         return False
 
     @property
-    def timeout_seconds(self):
+    def remaining(self):
+        """Return the number of seconds remaining until timeout.
+
+        Returns 1 if the timer is set to never expire.
+        """
+        # Timer never expires
+        if self.timeout is None:
+            return 1
+
+        # Timer hasn't started
+        if self._start_time is None:
+            return self.timeout
+
+        # Timer has started and hasn't been stopped
+        if self._end_time is None:
+            return self.timeout - (time.time() - self._start_time)
+
+        # Time has been start and been stopped
+        return self.timeout - (self._end_time - self._start_time)
+
+    def restart(self):
+        """Restart the timer."""
+        self.start()
+
+    def start(self):
+        """Resets and starts the timer running."""
+        self._start_time = time.time()
+        self._end_time = None
+
+    def stop(self):
+        """Stops the timer and resets it."""
+        self._end_time = time.time()
+
+    @property
+    def timeout(self):
         """Return the number of seconds set for timeout."""
-        return self._max_number_seconds
+        return self._timeout
 
-    @timeout_seconds.setter
-    def timeout_seconds(self, value):
+    @timeout.setter
+    def timeout(self, value):
         """Set the number of seconds before the timer expires.
 
         Parameters
         ----------
-        value : float or int or None
+        value : numeric or None
             The number of seconds before the timer expires. A value of None
-            means no timeout.
+            means the timer never expires.
         """
         # pylint: disable=attribute-defined-outside-init
-        self._max_number_seconds = value
-
-    @property
-    def time_remaining(self):
-        """Return the number of seconds remaining until timeout.
-
-        Returns -1 if the timer is set to unlimited timeout.
-        """
-        if self._start_time is None or self.timeout_seconds is None:
-            if self.timeout_seconds is None:
-                return -1
-
-            return self.timeout_seconds
-
-        seconds_elapsed = time.time() - self._start_time
-        return self.timeout_seconds - seconds_elapsed
+        self._timeout = value


Test patch:
diff --git a/pynetdicom/tests/test_assoc.py b/pynetdicom/tests/test_assoc.py
index 2bcbc8106..e6588a98d 100644
--- a/pynetdicom/tests/test_assoc.py
+++ b/pynetdicom/tests/test_assoc.py
@@ -1092,6 +1092,23 @@ class TestAssociationSendCEcho(object):
         assert assoc.is_released
         self.scp.stop()
 
+    def test_network_times_out(self):
+        """Regression test for #286."""
+        ae = AE()
+        ae.add_requested_context(VerificationSOPClass)
+        ae.add_supported_context(VerificationSOPClass)
+        scp = ae.start_server(('', 11112), block=False)
+
+        assoc = ae.associate('localhost', 11112)
+        assert assoc.is_established
+        assert assoc.network_timeout == 60
+        assoc.network_timeout = 0.5
+        assert assoc.network_timeout == 0.5
+
+        time.sleep(1.0)
+        assert assoc.is_aborted
+
+        scp.shutdown()
 
 class TestAssociationSendCStore(object):
     """Run tests on Assocation send_c_store."""
diff --git a/pynetdicom/tests/test_fsm.py b/pynetdicom/tests/test_fsm.py
index fdc216afb..dd396aa6f 100644
--- a/pynetdicom/tests/test_fsm.py
+++ b/pynetdicom/tests/test_fsm.py
@@ -958,7 +958,7 @@ class TestState01(TestStateBase):
         # Evt18: ARTIM timer expired from <local service>
         self.assoc._mode = "acceptor"
         self.assoc.acse_timeout = 0.05
-        self.assoc.dul.artim_timer.timeout_seconds = 0.05
+        self.assoc.dul.artim_timer.timeout = 0.05
         self.assoc.dul.artim_timer.start()
         self.assoc.start()
 
@@ -966,7 +966,7 @@ class TestState01(TestStateBase):
 
         self.assoc.kill()
 
-        assert self.assoc.dul.artim_timer.is_expired
+        assert self.assoc.dul.artim_timer.expired
 
         assert self.fsm._transitions == []
         assert self.fsm._changes == []
@@ -1465,7 +1465,7 @@ class TestState02(TestStateBase):
 
         time.sleep(0.1)
 
-        assoc.dul.artim_timer.timeout_seconds = 0.05
+        assoc.dul.artim_timer.timeout = 0.05
         assoc.dul.artim_timer.start()
 
         time.sleep(0.1)
@@ -2090,7 +2090,7 @@ class TestState03(TestStateBase):
         def _neg_as_acc(assoc):
             """Override ACSE._negotiate_as_acceptor so no A-ASSOCIATE (rsp)."""
             # Keep the state machine in Sta3 for 0.5 s
-            assoc.dul.artim_timer.timeout_seconds = 0.05
+            assoc.dul.artim_timer.timeout = 0.05
             assoc.dul.artim_timer.start()
             time.sleep(0.5)
             orig(assoc)
@@ -2715,7 +2715,7 @@ class TestState04(TestStateBase):
         self.assoc.dul.socket.connect = connect
         self.assoc.start()
         time.sleep(0.1)
-        self.assoc.dul.artim_timer.timeout_seconds = 0.05
+        self.assoc.dul.artim_timer.timeout = 0.05
         self.assoc.dul.artim_timer.start()
         time.sleep(0.1)
 
@@ -3222,7 +3222,7 @@ class TestState05(TestStateBase):
 
         self.assoc.start()
         time.sleep(0.1)
-        self.assoc.dul.artim_timer.timeout_seconds = 0.05
+        self.assoc.dul.artim_timer.timeout = 0.05
         self.assoc.dul.artim_timer.start()
         time.sleep(0.1)
 
@@ -3762,7 +3762,7 @@ class TestState06(TestStateBase):
 
         time.sleep(0.1)
 
-        self.assoc.dul.artim_timer.timeout_seconds = 0.05
+        self.assoc.dul.artim_timer.timeout = 0.05
         self.assoc.dul.artim_timer.start()
 
         time.sleep(0.2)
@@ -4338,7 +4338,7 @@ class TestState07(TestStateBase):
         self.assoc.start()
         time.sleep(0.1)
         self.assoc.dul.send_pdu(self.get_release(False))
-        self.assoc.dul.artim_timer.timeout_seconds = 0.05
+        self.assoc.dul.artim_timer.timeout = 0.05
         self.assoc.dul.artim_timer.start()
         time.sleep(0.1)
 
@@ -4973,7 +4973,7 @@ class TestState08(TestStateBase):
         self.assoc.start()
 
         time.sleep(0.1)
-        self.assoc.dul.artim_timer.timeout_seconds = 0.05
+        self.assoc.dul.artim_timer.timeout = 0.05
         self.assoc.dul.artim_timer.start()
 
         time.sleep(0.1)
@@ -5752,7 +5752,7 @@ class TestState09(TestStateBase):
         time.sleep(0.1)
         self.assoc.dul.send_pdu(self.get_release(False))
         time.sleep(0.1)
-        self.assoc.dul.artim_timer.timeout_seconds = 0.05
+        self.assoc.dul.artim_timer.timeout = 0.05
         self.assoc.dul.artim_timer.start()
         time.sleep(0.1)
 
@@ -6550,7 +6550,7 @@ class TestState10(TestStateBase):
 
         time.sleep(0.1)
 
-        assoc.dul.artim_timer.timeout_seconds = 0.05
+        assoc.dul.artim_timer.timeout = 0.05
         assoc.dul.artim_timer.start()
 
         time.sleep(0.1)
@@ -7394,7 +7394,7 @@ class TestState11(TestStateBase):
         time.sleep(0.1)
         self.assoc.dul.send_pdu(self.get_release(True))
         time.sleep(0.1)
-        self.assoc.dul.artim_timer.timeout_seconds = 0.05
+        self.assoc.dul.artim_timer.timeout = 0.05
         self.assoc.dul.artim_timer.start()
         time.sleep(0.1)
 
@@ -8194,7 +8194,7 @@ class TestState12(TestStateBase):
         time.sleep(0.1)
         assoc.dul.send_pdu(self.get_release(False))
         time.sleep(0.1)
-        assoc.dul.artim_timer.timeout_seconds = 0.05
+        assoc.dul.artim_timer.timeout = 0.05
         assoc.dul.artim_timer.start()
         time.sleep(0.1)
 
@@ -8894,7 +8894,7 @@ class TestState13(TestStateBase):
 
         time.sleep(0.1)
 
-        self.assoc.dul.artim_timer.timeout_seconds = 0.05
+        self.assoc.dul.artim_timer.timeout = 0.05
         self.assoc.dul.artim_timer.start()
 
         time.sleep(0.1)
diff --git a/pynetdicom/tests/test_timer.py b/pynetdicom/tests/test_timer.py
index 5a7556121..c437a6357 100644
--- a/pynetdicom/tests/test_timer.py
+++ b/pynetdicom/tests/test_timer.py
@@ -16,39 +16,39 @@ class TestTimer(object):
     def test_init(self):
         """Test Timer initialisation"""
         timer = Timer(10)
-        assert timer.timeout_seconds == 10
+        assert timer.timeout == 10
         timer = Timer(0)
-        assert timer.timeout_seconds == 0
+        assert timer.timeout == 0
         timer = Timer(None)
-        assert timer.timeout_seconds is None
+        assert timer.timeout is None
 
     def test_property_setters_getters(self):
         """Test Timer property setters and getters."""
         timer = Timer(0)
-        assert timer.timeout_seconds == 0
-        assert not timer.is_expired
-        assert timer.time_remaining == 0
+        assert timer.timeout == 0
+        assert not timer.expired
+        assert timer.remaining == 0
 
         timer = Timer(None)
-        assert timer.timeout_seconds is None
-        assert not timer.is_expired
-        assert timer.time_remaining == -1
+        assert timer.timeout is None
+        assert not timer.expired
+        assert timer.remaining == 1
 
-        timer.timeout_seconds = 10
-        assert timer.timeout_seconds == 10
-        assert not timer.is_expired
-        assert timer.time_remaining == 10
+        timer.timeout = 10
+        assert timer.timeout == 10
+        assert not timer.expired
+        assert timer.remaining == 10
 
-        timer.timeout_seconds = 0.2
+        timer.timeout = 0.2
         timer.start()
         time.sleep(0.1)
-        assert timer.time_remaining < 0.1
-        assert not timer.is_expired
+        assert timer.remaining < 0.1
+        assert not timer.expired
         time.sleep(0.1)
-        assert timer.is_expired
+        assert timer.expired
 
-        timer.timeout_seconds = None
-        assert timer.timeout_seconds is None
+        timer.timeout = None
+        assert timer.timeout is None
 
     def test_start_stop(self):
         """Test Timer stops."""
@@ -57,15 +57,66 @@ class TestTimer(object):
         time.sleep(0.1)
         timer.stop()
         time.sleep(0.2)
-        assert not timer.is_expired
+        assert not timer.expired
 
     def test_restart(self):
         """Test Timer restarts correctly."""
         timer = Timer(0.2)
         timer.start()
         time.sleep(0.1)
+        assert timer.expired is False
         timer.restart()
         time.sleep(0.15)
-        assert not timer.is_expired
-        time.sleep(0.05)
-        assert timer.is_expired
+        assert timer.expired is False
+        time.sleep(0.1)
+        assert timer.expired is True
+
+    def test_no_timeout(self):
+        """Test the timer with no time out."""
+        timer = Timer(None)
+        assert timer.timeout is None
+        assert timer.expired is False
+        assert timer.remaining is 1
+        timer.start()
+        assert timer.expired is False
+        assert timer.remaining is 1
+        time.sleep(0.5)
+        assert timer.expired is False
+        assert timer.remaining is 1
+        timer.stop()
+        assert timer.expired is False
+        assert timer.remaining is 1
+
+    def test_timeout(self):
+        """Test the timer with a time out."""
+        timer = Timer(0.1)
+        assert timer.timeout == 0.1
+        assert timer.expired is False
+        assert timer.remaining == 0.1
+        timer.start()
+        assert timer.expired is False
+        assert timer.remaining > 0
+        time.sleep(0.2)
+        assert timer.expired is True
+        assert timer.remaining < 0
+        timer.stop()
+        assert timer.expired is True
+        assert timer.remaining < 0
+
+    def test_timeout_stop(self):
+        """Test stopping the timer."""
+        timer = Timer(0.1)
+        assert timer.timeout == 0.1
+        assert timer.expired is False
+        assert timer.remaining == 0.1
+        timer.start()
+        timer.stop()
+        assert timer.timeout == 0.1
+        assert timer.expired is False
+        assert timer.remaining > 0
+        timer.start()
+        time.sleep(0.2)
+        timer.stop()
+        assert timer.timeout == 0.1
+        assert timer.expired is True
+        assert timer.remaining < 0