Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lazy initialize Net::LDAP::Connection's internal socket #235

Merged
merged 12 commits into from
Jan 11, 2016

Conversation

jch
Copy link
Member

@jch jch commented Nov 10, 2015

This PR's goal is to break the dependency between Net::LDAP::Connection#initialize and socket initialization. This would fix the testing code smell of stubbing out opening a TCPSocket whenever we initialize a connection object.

  • add private #socket and use it for all socket communication
  • remove socket initialization from constructor

I'm looking for early feedback as this is a work in progress because many existing tests expect the constructor to initialize the socket. I don't expect this to break the public API since Net::LDAP::Connection is an internal class.

cc @satoryu @javanthropus since I recall one of you asking for a better way to test this.

@javanthropus
Copy link

@jch It might be better to allow the socket class to be set via the initialize method of Net::LDAP::Connection along with the rest of the parameters rather than setting it via an additional call to an attribute modifier after initialization. Doing so would simplify utilizing alternative socket classes generally, not just in tests, while also keeping the overall interface for the Connection class relatively simple.

@jch
Copy link
Member Author

jch commented Nov 17, 2015

@javanthropus the Connection class does take a socket parameter. However, I don't want to expose this as a public method on Net::LDAP. It doesn't make sense for users to think about messing with sockets.

Net::LDAP::Connection.new(:hosts => hosts)
connection = Net::LDAP::Connection.new(:hosts => hosts)
connection.socket_class = FakeTCPSocket
connection.socket

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jch Uses such as these 3 lines are what I'm proposing to simplify a bit. I think it would look better and be easier to use like this:

connection = Net::LDAP::Connection.new(:hosts => hosts, :socket_class => FakeTCPSocket)

The idea is for this single call to create the connection and arrange for the opening of the socket using the specified socket class when needed all in a single go rather than requiring the socket method to be called to actually open the socket. If the socket class is not specified, it should default to TCPSocket.

This is a fairly common way to handle dependency injection as you're trying to enable here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, with this change in place, you wouldn't need to add the socket_class and socket_class= attributes to the class definition of Connection at all.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javanthropus ++ your idea sounds good 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javanthropus ah, I totally misunderstood! Parameterizing the socket class rather than the socket instance is a good idea. I'll roll with that.

@satoryu
Copy link
Collaborator

satoryu commented Nov 17, 2015

ah, this test for 1.9.3 support failed since byebug supports 2.0.0 or later.
There are two ways,

  1. bye 1.9.3 support
  2. bye byebug 🐛

IMO, the option 1 looks better as Support for Ruby 1.9.3 has been ended.

@jch
Copy link
Member Author

jch commented Nov 17, 2015

@satoryu nice investigation. 👍 to dropping support for 1.9 series, but let's do that in a separate PR. Would you be interested in opening one up?

@satoryu
Copy link
Collaborator

satoryu commented Dec 3, 2015

This PR will pass all tests after the master is merged 👍

@jch
Copy link
Member Author

jch commented Dec 3, 2015

@satoryu thanks for the update. I've been busy with work, so haven't had time to finish this up.

@jch
Copy link
Member Author

jch commented Jan 8, 2016

@satoryu @javanthropus this is ready for another pass if either of you could spare some time.

#
# Typically a TCPSocket, but can be a OpenSSL::SSL::SSLSocket
def socket
return @conn if defined? @conn
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, I have a question.
Why is defined? used? it looks working as if @conn. but there are a little bit difference between the both.

@foo = 1 # <= defined instance variable
puts 'ok' if @foo # => 'ok'
puts 'ok' if defined? @foo # => 'ok'   so the both are same.

@bar = nil # defined but its value is nil
puts 'ok' if @bar # => nothing
puts 'ok' if defined @bar # => 'ok'  so different behavior

I believe that this difference doesn't affect on the behavior of Net::LDAP.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@satoryu This is likely done to avoid warnings from the interpreter for using uninitialized instance variables. The defined? method is exempted from the warnings as I recall.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jch
Copy link
Member Author

jch commented Jan 11, 2016

Thanks for the review!

jch added a commit that referenced this pull request Jan 11, 2016
Lazy initialize Net::LDAP::Connection's internal socket
@jch jch merged commit b32f4e7 into master Jan 11, 2016
This was referenced Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants