Patch resolving the timeout issue on lost conection #439

Closed
vazir opened this Issue Nov 14, 2014 · 4 comments

Projects

None yet

3 participants

@vazir
vazir commented Nov 14, 2014

The patch below resolves a timeout issue, when one have an established session, and connection to the destination disappears (I tested by switching off the wlan interface) - without this patch the exec_command will hang on trying to open a channel (transport - open_channel) since there is an infinite loop waiting for an event. This adds the timeout to the corresponding code, with raising an SSHException on timeout.
this patch is against the paramico trunk of the 14th Nov 2014 - 838ab5b

diff -ur paramiko-master-orig/paramiko/client.py paramiko-master-patched/paramiko/client.py
--- paramiko-master-orig/paramiko/client.py     2014-11-13 00:58:28.000000000 +0300
+++ paramiko-master-patched/paramiko/client.py  2014-11-14 17:08:11.000000000 +0300
@@ -338,7 +338,7 @@

         :raises SSHException: if the server fails to execute the command
         """
-        chan = self._transport.open_session()
+        chan = self._transport.open_session(timeout=timeout)
         if get_pty:
             chan.get_pty()
         chan.settimeout(timeout)
diff -ur paramiko-master-orig/paramiko/transport.py paramiko-master-patched/paramiko/transport.py
--- paramiko-master-orig/paramiko/transport.py  2014-11-13 00:58:28.000000000 +0300
+++ paramiko-master-patched/paramiko/transport.py       2014-11-14 17:30:44.352597773 +0300
@@ -587,7 +587,7 @@
         """
         return self.active

-    def open_session(self, window_size=None, max_packet_size=None):
+    def open_session(self, window_size=None, max_packet_size=None, timeout=None):
         """
         Request a new channel to the server, of type ``"session"``.  This is
         just an alias for calling `open_channel` with an argument of
@@ -612,7 +612,8 @@
         """
         return self.open_channel('session',
                                  window_size=window_size,
-                                 max_packet_size=max_packet_size)
+                                 max_packet_size=max_packet_size,
+                                 timeout=timeout)

     def open_x11_channel(self, src_addr=None):
         """
@@ -659,7 +660,8 @@
                      dest_addr=None,
                      src_addr=None,
                      window_size=None,
-                     max_packet_size=None):
+                     max_packet_size=None,
+                     timeout=None):
         """
         Request a new channel to the server. `Channels <.Channel>` are
         socket-like objects used for the actual transfer of data across the
@@ -683,17 +685,20 @@
             optional window size for this session.
         :param int max_packet_size:
             optional max packet size for this session.
+        :param float timeout:
+            optional timeout opening a channel, default 3600s (1h)

         :return: a new `.Channel` on success

-        :raises SSHException: if the request is rejected or the session ends
-            prematurely
+        :raises SSHException: if the request is rejected, the session ends
+            prematurely or there is a timeout openning a channel

         .. versionchanged:: 1.15
             Added the ``window_size`` and ``max_packet_size`` arguments.
         """
         if not self.active:
             raise SSHException('SSH session not active')
+        timeout = 3600 if timeout is None else timeout
         self.lock.acquire()
         try:
             window_size = self._sanitize_window_size(window_size)
@@ -722,6 +727,7 @@
         finally:
             self.lock.release()
         self._send_user_message(m)
+        start_ts = time.time()
         while True:
             event.wait(0.1)
             if not self.active:
@@ -731,6 +737,8 @@
                 raise e
             if event.isSet():
                 break
+            elif start_ts + timeout < time.time():
+                raise SSHException('Timeout openning channel.')
         chan = self._channels.get(chanid)
         if chan is not None:
             return chan
@bitprophet
Member

Pro tip, if you aren't able to submit an actual pull request (see the site help - github's help is pretty well written :)), it's best to use the right formatting when submitting patches, otherwise it's hard to read :D I edited your comment to add code blocks and a 'diff' type selector.

Anyway, thanks for this, will try to verify on my end. Note to self, these other tickets may be related: #62, #86 (definitely looks like a dupe).

@vazir
vazir commented Nov 15, 2014

This one: #86 looks like doing the same, I did ;) The #62 looks different by description, as the behavior I encountered is when the connection is already on for a while, say you would like to execute many commands after a while, and keep connection open for further requests.

@fjolliton

We stumbled too on this problem recently, and we circumvented it by wrapping the risky calls with ALARM signal, which is sad.

It would be nice to resolve this issue, because it is a big problem in production.

@lndbrg lndbrg added a commit to lndbrg/paramiko that referenced this issue Feb 24, 2015
@lndbrg lndbrg Patch resolving the timeout issue on lost conection.
(This rolls in patch in #439)
6ba6ccd
@lndbrg lndbrg referenced this issue Feb 24, 2015
Merged

Timeout fixes #491

@bitprophet bitprophet added this to the 1.13.4 / 1.14.3 / 1.15.3 milestone Feb 27, 2015
@bitprophet
Member

Rolling into #491

@bitprophet bitprophet closed this Feb 27, 2015
@bitprophet bitprophet added a commit that referenced this issue Sep 30, 2015
@bitprophet bitprophet Rework changelog entries re #491 a bit
Closes #491, closes #62, closes #439
57106d0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment