Removed Sometimes Unnecessary String Instantiation That Can Cause Thread Blocking #23
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… from a result set could cause threads to back up in a synchronization queue. Calling #getBytes(...) on the result set makes a call to #getValueObject(...), which calls AbstractValueObject#isNull(). The existing workflow was as follows: 1) Declare 'rawValue' and set it to the return value of #getString() <- This can cause issues as described below 2) Declare 'zeroTimestamp' and set it 3) Declare 'zeroDate' and set it 4.1) If 'rawBytes' is null, return true 4.2) If the datatype is either TIMESTAMP or DATETIME, check for equality between 'rawValue' and 'zeroTimestamp'. If true, return true 4.3) If the datatype is DATE, check for equality between 'rawValue' and 'zeroDate'. If true, return true 4.4) Otherwise, return false In this workflow, we make a call to #getString() whether we end up needing its return value or not. Calling #getString() executes a chain of method calls that eventually end up at java.nio.charset.CoderResult$Cache.get(int). In the middle of this call stack is a call from java.lang.StringCoding to a statically referenced instance of java.lang.StringCoding$StringDecoder. Transitively, this means that it ends up using the same instance of java.nio.charset.CoderResult$Cache. Since $Cache#get(int) is synchronized, this can cause threads to back up in a high-traffic environment. This issue is exacerbated by the fact that it is run within the context of an open database connection. If using database connection pooling, enough threads can pile up in the synchronization pool that the database connection pool is effectively exhausted. Requests for database connections can continue to enqueue until the JVM exhausts its heap and becomes unstable. Since the return value of #getString() is only used when the data type is DATE, DATETIME, or TIMESTAMP, its invocation is unnecessary in other cases. Instead of calling it unconditionally, I replaced the occurrences of 'rawValue' in the conditions with calls to #getString(). Since the data type will either be DATE -or- (DATETIME or TIMESTAMP), it would not be possible for the invocation to occur twice as that condition would short-circuit the && condition of zeroXXX.equals(rawValue).
|
well spotted ! |
rusher
added a commit
that referenced
this pull request
Jul 23, 2015
Removed Sometimes Unnecessary String Instantiation That Can Cause Thread Blocking
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolved an issue where several concurrent reads of large byte arrays from a result set could cause threads to back up in a synchronization queue.
Calling #getBytes(...) on the result set makes a call to #getValueObject(...), which calls AbstractValueObject#isNull(). The existing workflow was as follows:
4.1) If 'rawBytes' is null, return true
4.2) If the datatype is either TIMESTAMP or DATETIME, check for equality between 'rawValue' and 'zeroTimestamp'. If true, return true
4.3) If the datatype is DATE, check for equality between 'rawValue' and 'zeroDate'. If true, return true
4.4) Otherwise, return false
In this workflow, we make a call to #getString() whether we end up needing its return value or not.
Calling #getString() executes a chain of method calls that eventually end up at java.nio.charset.CoderResult$Cache.get(int). In the middle of this call stack is a call from java.lang.StringCoding to a statically referenced instance of java.lang.StringCoding$StringDecoder. Transitively, this means that it ends up using the same instance of java.nio.charset.CoderResult$Cache. Since $Cache#get(int) is synchronized, this can cause threads to back up in a high-traffic environment.
This issue is exacerbated by the fact that it is run within the context of an open database connection. If using database connection pooling, enough threads can pile up in the synchronization pool that the database connection pool is effectively exhausted. Requests for database connections can continue to enqueue until the JVM exhausts its heap and becomes unstable.
Since the return value of #getString() is only used when the data type is DATE, DATETIME, or TIMESTAMP, its invocation is unnecessary in other cases. Instead of calling it unconditionally, I replaced the occurrences of 'rawValue' in the conditions with calls to #getString(). Since the data type will either be DATE -or- (DATETIME or TIMESTAMP), it would not be possible for the invocation to occur twice as that condition would short-circuit the && condition of zeroXXX.equals(rawValue).