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.

Sign up for GitHub

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

Jump to bottom

Fix for static logger member in abstract class 'SQLServerClobBase' #537

Merged
merged 1 commit into from
Nov 15, 2017
Merged

Fix for static logger member in abstract class 'SQLServerClobBase' #537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions src/main/java/com/microsoft/sqlserver/jdbc/SQLServerClob.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,6 @@
public class SQLServerClob extends SQLServerClobBase implements Clob {
private static final long serialVersionUID = 2872035282200133865L;

// Loggers should be class static to avoid lock contention with multiple threads
private static final Logger logger = Logger.getLogger("com.microsoft.sqlserver.jdbc.internals.SQLServerClob");

/**
* Create a new CLOB
*
Expand All @@ -50,19 +47,19 @@ public class SQLServerClob extends SQLServerClobBase implements Clob {
@Deprecated
public SQLServerClob(SQLServerConnection connection,
String data) {
super(connection, data, (null == connection) ? null : connection.getDatabaseCollation(), logger, null);
super(connection, data, (null == connection) ? null : connection.getDatabaseCollation(), null);

if (null == data)
throw new NullPointerException(SQLServerException.getErrString("R_cantSetNull"));
}

SQLServerClob(SQLServerConnection connection) {
super(connection, "", connection.getDatabaseCollation(), logger, null);
super(connection, "", connection.getDatabaseCollation(), null);
}

SQLServerClob(BaseInputStream stream,
TypeInfo typeInfo) throws SQLServerException, UnsupportedEncodingException {
super(null, stream, typeInfo.getSQLCollation(), logger , typeInfo);
super(null, stream, typeInfo.getSQLCollation(), typeInfo);
}

final JDBCType getJdbcType() {
Expand Down Expand Up @@ -91,7 +88,9 @@ abstract class SQLServerClobBase implements Serializable {
private ArrayList<Closeable> activeStreams = new ArrayList<>(1);

transient SQLServerConnection con;
private static Logger logger;

private final Logger logger = Logger.getLogger(getClass().getName());
Copy link
Contributor

Choose a reason for hiding this comment

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

How about this comment that got deleted?
// Loggers should be class static to avoid lock contention with multiple threads

Copy link
Member Author

Choose a reason for hiding this comment

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

Well - its because this logger object won't be static in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is now a logger lookup whenever an instance of SQLServerClob or SQLServerNClob is created. This could be avoided if:

  1. there is a static final logger in SQLServerClob or SQLServerNClob, like before
  2. the logger in SQLServerClobBase is made private final, like in this PR
  3. SQLServerClob and SQLServerNClob pass the logger in the super constructor call

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @marschall

Thanks for replying back! We could go for either solution, but I removed the loggers from sub classes as:

  1. Class Lookup isn't expensive here.
  2. The loggers in sub classes are not used anywhere else except being passed in constructor (but remain attached in memory with class objects)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not the class lookup that I was concerned about, it's get logger lookup.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you please explain what you mean by Logger lookup and how will it be different in the other case - maybe I am not able to understand your point.

Copy link
Contributor

Choose a reason for hiding this comment

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

With a non-static logger in SQLServerClob or SQLServerNClob every time a new instance of these classes is generated the following is executed:

Logger.getLogger(getClass().getName())

If we ignore getClass().getName() for now that still leaves us with Logger.getLogger() which does among other things:

  1. Reflection.getCallerClass() which creates an exception and walks the entire stack
  2. check for a security manager several times
  3. goes through a global lock in java.util.logging.LogManager.LoggerContext#namedLoggers

If you make the loggers in SQLServerClob and SQLServerNClob static all that work happens only one during class loading.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @marschall I understand the concern for non-static logger here. Made changes in the new PR #563 as discussed above. Request you to review the PR so we can get it working.


final private String traceID = getClass().getName().substring(1 + getClass().getName().lastIndexOf('.')) + ":" + nextInstanceID();

final public String toString() {
Expand Down Expand Up @@ -129,7 +128,6 @@ private String getDisplayClassName() {
SQLServerClobBase(SQLServerConnection connection,
Object data,
SQLCollation collation,
Logger logger,
TypeInfo typeInfo) {
this.con = connection;
if (data instanceof BaseInputStream) {
Expand All @@ -140,7 +138,6 @@ private String getDisplayClassName() {
}
this.sqlCollation = collation;
this.typeInfo = typeInfo;
SQLServerClobBase.logger = logger;

if (logger.isLoggable(Level.FINE)) {
String loggingInfo = (null != connection) ? connection.toString() : "null connection";
Expand Down
11 changes: 5 additions & 6 deletions src/main/java/com/microsoft/sqlserver/jdbc/SQLServerNClob.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,23 +10,22 @@

import java.io.UnsupportedEncodingException;
import java.sql.NClob;
import java.util.logging.Logger;

/**
* SQLServerNClob represents a National Character Set LOB object and implements java.sql.NClob.
*/

public final class SQLServerNClob extends SQLServerClobBase implements NClob {
// Loggers should be class static to avoid lock contention with multiple threads
private static final Logger logger = Logger.getLogger("com.microsoft.sqlserver.jdbc.internals.SQLServerNClob");

SQLServerNClob(SQLServerConnection connection) {
super(connection, "", connection.getDatabaseCollation(), logger, null);
private static final long serialVersionUID = 1L;

SQLServerNClob(SQLServerConnection connection) {
super(connection, "", connection.getDatabaseCollation(), null);
}

SQLServerNClob(BaseInputStream stream,
TypeInfo typeInfo) throws SQLServerException, UnsupportedEncodingException {
super(null, stream, typeInfo.getSQLCollation(), logger , typeInfo);
super(null, stream, typeInfo.getSQLCollation(), typeInfo);
}

final JDBCType getJdbcType() {
Expand Down