Sftp type error #49363

[Robey Pointer]

On 16 Jun 2006, at 16:05, Aaron Bentley wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Robey Pointer wrote:
>>      def stop(self):
>> -        self.stop_event.set()
>> +        self._stop_event.set()
>>          # use a timeout here, because if the test fails, the  
>> server thread may
>>          # never notice the stop_event.
>>          self.join(5.0)
>> +        self._socket.close()
>
> So threads can't join themseves, but stop isn't mean to be called  
> within
> the thread, so it's okay?  This looks correct, but a comment might be
> useful.

Okay, added.


>> +            try:
>> +                s, addr_unused = self._socket.accept()
>> +                # because the loopback socket is inline, and  
>> transports are
>> +                # never explicitly closed, best to launch a new  
>> thread.
>> +                threading.Thread(target=self._callback, args= 
>> (s,)).start()
>> +            except socket.error, x:
>> +                pass #Ignore socket errors
>
> I think socket errors could indicate a serious problem.  Is it  
> necessary
> to ignore all of them?

This and the next chunk were just moved around from existing code.   
I'll change it to log something about the socket error.


>
>> +            except Exception, x:
>> +                # probably a failed test
>> +                sys.excepthook(*sys.exc_info())
>> +                warning('Exception from within unit test server  
>> thread: %r' % x)
>
> This looks overbroad.  Couldn't we get SystemExit?  Wouldn't it be
> better to let these exceptions propogate?

The exception will be eaten shortly afterwards as the thread dies.   
When this exception is hit (and I've hit it a lot while hacking :),  
the primary unit test thread will give the actual failure or error  
message.  The exception logging within the "stub server" thread is  
just useful info for debugging.  I'll clarify that in the comment.


>> class SFTPServer(Server):
>> @@ -937,10 +947,12 @@
>>          """StubServer uses this to log when a new server is  
>> created."""
>>          self.logs.append(message)
>>
>> -    def _run_server(self, s, stop_event):
>> +    def _run_server(self, s):
>>          ssh_server = paramiko.Transport(s)
>>          key_file = os.path.join(self._homedir, 'test_rsa.key')
>> -        file(key_file, 'w').write(STUB_SERVER_KEY)
>> +        f = open(key_file, 'w')
>> +        f.write(STUB_SERVER_KEY)
>> +        f.close()
>
> Apparently, this is the intended use: file is the object type and open
> is the constructor people should use.  Should we start using this  
> style?

I only recently found this out.  My main purpose here was to make the  
close() explicit, since there was a thread a few months ago where the  
consensus seemed to be that the implicit-close was bad form.


> The concept looks okay, but I'd like to understand the exception
> swallowing better before voting.

New patch attached.

robey


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: listener-patch.txt
Url: https://lists.ubuntu.com/archives/bazaar/attachments/20060617/7e25d943/attachment.txt