Respect connect_timeout when establishing SSL connections #273
Conversation
tmm1
force-pushed
the
ssl-connect-timeout
branch
from
22fa81c to
a742ffe
Compare
tmm1
force-pushed
the
ssl-connect-timeout
branch
from
a742ffe to
09d0c36
Compare
tmm1
force-pushed
the
ssl-connect-timeout
branch
from
d192a85 to
b5b6d5a
Compare
| if IO.select([conn], nil, nil, timeout) | ||
| retry | ||
| else | ||
| raise Net::LDAP::LdapError, "OpenSSL connection read timeout" |
There was a problem hiding this comment.
Net::LDAP::LdapError is deprecated so please use Net::LDAP::Error instead.
I as a user of net-ldap want new exception class for timeout to let us handle cases where connection timeout happens. lib/net/ldap/error.rb would help you, I think.
There was a problem hiding this comment.
I switched to SocketError, but I can add a new TimeoutError or return ETIMEDOUT if that makes more sense.
There was a problem hiding this comment.
Sounds good, switched to Errno::ETIMEDOUT
|
@tmm1 may i ask what prompted this PR? I recently ran into issues when using puma (rather than unicorn) for an application which uses LDAP which is resulting in timeouts in the socket#connect method when binding and searching. hoping this is related and could fix our issue! |
| @@ -291,7 +291,7 @@ def test_queued_read_setup_encryption_with_start_tls | |||
| and_return(result2) | |||
| mock.should_receive(:write) | |||
| conn = Net::LDAP::Connection.new(:socket => mock) | |||
| flexmock(Net::LDAP::Connection).should_receive(:wrap_with_ssl).with(mock, {}). | |||
| flexmock(Net::LDAP::Connection).should_receive(:wrap_with_ssl).with(mock, {}, nil). | |||
There was a problem hiding this comment.
@tmm1 Doesn't this mocking cover your changes on wrap_with_ssl, do ti? If you tested your changes work as you expected, could you share how to do it?
I know it's really hard to reproduce the situation timeout or something network issues happens.
There was a problem hiding this comment.
I tested the change by setting a very low timeout on LDAPS connection and
make sure an exception is raised.
On Mon, Jun 20, 2016 at 10:15 PM Tatsuya Sato [email protected]
wrote:
In test/test_ldap_connection.rb
#273 (comment)
:@@ -291,7 +291,7 @@ def test_queued_read_setup_encryption_with_start_tls
and_return(result2)
mock.should_receive(:write)
conn = Net::LDAP::Connection.new(:socket => mock)
- flexmock(Net::LDAP::Connection).should_receive(:wrap_with_ssl).with(mock, {}).
- flexmock(Net::LDAP::Connection).should_receive(:wrap_with_ssl).with(mock, {}, nil).
@tmm1 https://github.com/tmm1 Doesn't this mocking cover your changes
on wrap_with_ssl, do ti? If you tested your changes work as you expected,
could you share how to do it?I know it's really hard to reproduce the situation timeout or something
network issues happens.—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/ruby-ldap/ruby-net-ldap/pull/273/files/749c22b4e5514ead10c92bcaec1c5a1eb49db455#r67809160,
or mute the thread
https://github.com/notifications/unsubscribe/AAAKB01CFZ0ewSejsl4qpinzGhwx48U5ks5qN3N4gaJpZM4I25cW
.
Sounds like you need to set a custom |
=== Net::LDAP 0.15.0
* Respect connect_timeout when establishing SSL connections {#273}[ruby-ldap/ruby-net-ldap#273]
cc #243