|| instead of 'or' (local style), spaces after # in comments, wrap long lines, keep the => lined up in the constants at top, use logging system instead of literal "warn"?
If you could also upload this patch once updated to codereview.appspot.com, that'd be great. (there's a command-line tool you can download from there which does it all... no need to use the web-app) Brad On Mon, Jun 16, 2008 at 8:19 AM, Jacob Burkhart <[EMAIL PROTECTED]> wrote: > Hi, > Can I get this patch in to djabberd? > > Incrementing a counter on empty SSL reads is not the correct way to > determine ssl_eof > (http://code.sixapart.com/trac/djabberd/changeset/758) > > Instead we should check the error code to see if it is one of the SSL > errors for which we are supposed to retry( > http://www.openssl.org/docs/ssl/SSL_get_error.html) > > thanks, > Jacob > > > > On Thu, Feb 28, 2008 at 12:40 PM, Jacob Burkhart <[EMAIL PROTECTED]> > wrote: > >> looks like the number was original 3, and then bumped up to 10 because of >> observed disconnects "in practice" >> http://code.sixapart.com/trac/djabberd/changeset/758 >> >> >> On Thu, Feb 28, 2008 at 11:33 AM, Jacob Burkhart <[EMAIL PROTECTED]> >> wrote: >> >>> Hi, >>> So we've been experiencing some bizarre behavior with clients randomly >>> disconnecting when trying to send messages over SSL. The disconnects vary >>> based on size of message and speed of network connections. >>> >>> The problems seems to stem from: >>> >>> http://code.sixapart.com/trac/djabberd/browser/trunk/DJabberd/lib/DJabberd/Connection.pm#L494 >>> >>> In http://code.sixapart.com/trac/djabberd/changeset/756 bradfitz added >>> code to handle non-graceful SSL disconnects. However, the number of times >>> to retry read before giving up (10) proved to be 1 to 2 too few times in our >>> particular situation. >>> >>> But why did he pick 10? >>> >>> In our tests, the number of times that Net::SSLeay::read can return zero >>> bytes in a row varies based on the speed of a client's connection and the >>> size of bytes attempting to be sent. >>> >>> During stream initiation for instance, we experience about 2 reads of >>> zero in a row from clients connected on reasonably fast connections, and 0 >>> reads of zero in a row from a client running on the same machine as the >>> server. >>> >>> When sending messages larger than 8k, the number of zero reads is >>> somewhere under 10 for clients connection over local ethernet, but is in the >>> 11 to 12 range for clients connecting over WiFi. >>> >>> So clearly, counting the number of zero bytes reads that happen is a row >>> is not the most reliable way to determine if the client has improperly >>> disconnected >>> >>> It turns out that if we look at the Djabberd code for SSL write, we can >>> see that there is a different answer for handling write failures. >>> >>> >>> http://code.sixapart.com/trac/djabberd/browser/trunk/DJabberd/lib/DJabberd/Stanza/StartTLS.pm#L90 >>> >>> Here, get_error is called to determine if the problem might have been >>> caused by SSL_ERROR_WANT_READ or perhaps SSL_ERROR_WANT_WRITE. If it is, >>> then this is not a fatal condition, and we don't want to close the >>> connection. instead, we simply return zero and expect that the writer will >>> come around and try again at some point. >>> >>> So why not do the same thing for read failures? >>> >>> Please consider my patch (attached) as a possible solution for this >>> problem. >>> >>> thanks, >>> Jacob >>> >>> >>> >>> >>> >>> >>> >> >