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

Added connection properties to specify custom SocketFactory #1217

Merged
merged 13 commits into from
Mar 31, 2020

Conversation

sehrope
Copy link
Contributor

@sehrope sehrope commented Dec 23, 2019

Adds a new connection property, socketFactoryClass, to allow overriding the SocketFactory used to create Sockets. Similar to the custom trust manager, there's also an optional string property, socketFactorConstructorArg, to pass as a constructor arg to further customize the SocketFactory. If not specified then the system default via SocketFactory.getDefault() is used.

First commit adds an internal helper for instantiating custom class instances as the logic is the same as for the custom trust manager.

Second commit has the new feature and adds a test that configures a basic SocketFactory that keeps logs its usage (to test that it's actually being used).

The SocketFactory is created once for each connection and cached in private variable. It may be used multiple times if the driver attempts to concurrently open sockets (ex: via multiple address resolution for the DB host). My reading of that code suggests that no thread synchronization is required as the sockets are created by the parent prior to executing in their own threads. Would be good for another set of eyes to confirm: https://github.com/microsoft/mssql-jdbc/blob/dev/src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java#L2638-L2654

@rene-ye
Copy link
Member

rene-ye commented Dec 24, 2019

Is this PR for a particular issue or is it just an overall improvement suggestion?

@sehrope
Copy link
Contributor Author

sehrope commented Dec 24, 2019

I haven't opened a separate issue for it but I can if it helps track things. I'd like to be able to customize the Socket created internally by the driver.

Without this PR there is no way to do so as the driver issues a new Socket() to directly create its sockets (which end up using the default socket factory). This PR allows customizing the socket creation by specifying a custom class to act as the socket factory for that one connection without having to override the default JDK-wide socket factory.

FYI, it's a common option on most Java drivers (both JDBC and non-JDBC):

@rene-ye
Copy link
Member

rene-ye commented Jan 9, 2020

Just a quick update to this PR. The team will need some time to review the contents of this PR, but it will probably start no earlier than early February as our bandwidth is currently limited.

Adds a Util.newInstance(...) helper to instantiate a class by name with
an optional argument and replaces similar code to create a custom
TrustManager with the new helper.
Adds a connection property, socketFactoryClass, to override the SocketFactory
used to create Sockets for driver connections. If unspecified, the default, then
the system default SocketFactory.getDefault() will be used. If a class name is
specified then it must be assignable to javax.net.SocketFactory.

An optional argument can be specified via the property socketFactoryConstructorArg.
If specified then the single String argument constructor of the class will be
invoked. Otherwise the no-argument constructor will be invoked.
@sehrope sehrope force-pushed the feat-add-custom-socket-factory branch from b66f063 to 4c40a55 Compare January 10, 2020 12:28
@sehrope
Copy link
Contributor Author

sehrope commented Jan 10, 2020

@rene-ye Thanks for the update.

@sehrope
Copy link
Contributor Author

sehrope commented Jan 10, 2020

Rebased to resolve some merge conflicts on dev, fixed a typo, and added one more test for invalid class values (similar to the one for the trust manager property).

@ulvii ulvii added this to the 8.2.2 milestone Feb 27, 2020
rene-ye
rene-ye previously approved these changes Mar 11, 2020
peterbae
peterbae previously approved these changes Mar 23, 2020
lilgreenbird
lilgreenbird previously approved these changes Mar 26, 2020
@ulvii ulvii dismissed stale reviews from lilgreenbird, peterbae, and rene-ye via f051f95 March 28, 2020 00:15
peterbae
peterbae previously approved these changes Mar 28, 2020
…at-add-custom-socket-factory

# Conflicts:
#	src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java
peterbae
peterbae previously approved these changes Mar 31, 2020
lilgreenbird
lilgreenbird previously approved these changes Mar 31, 2020
@peterbae peterbae dismissed stale reviews from lilgreenbird and themself via d3c86a3 March 31, 2020 18:07
peterbae
peterbae previously approved these changes Mar 31, 2020
@ulvii
Copy link
Contributor

ulvii commented Mar 31, 2020

Hi @sehrope ,
We made a few changes to your branch around error messages and file formatting. Merging the PR, appreciate the contribution. 👍

@ulvii ulvii merged commit d30f340 into microsoft:dev Mar 31, 2020
@ulvii ulvii changed the title Feat add custom socket factory Added connection properties to specify custom SocketFactory Mar 31, 2020
@sehrope
Copy link
Contributor Author

sehrope commented Apr 2, 2020

Nice. Thanks for the review and merging.

@ulvii ulvii added the Public API Changes in Public API label Jul 29, 2020
@zxcvboy
Copy link

zxcvboy commented Oct 28, 2020

Hi. @sehrope

Is this property(socketFactoryClass) ready to use? My jdbc driver`s version is "9.1.0.jre8-preview". 
I tried to use custom SocketFactory, however it seemed not be adopted by DriverManager. 
Below is my connection url.
"jdbc:sqlserver://testdb:1433;databaseName=LIFERAY_TEST;user=MEHOLiferaySearch;password=1qaz@WSX;socketFactoryClass=customer.capjdbc.DummySocketFactory;socketFactoryConstructorArg=TEST-CUSTOM-ARG"

Thanks a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Public API Changes in Public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants