Changed logging logic in AE and improved performance #773
Conversation
| try { | ||
| // AES encryption CBC mode and PKCS5 padding | ||
| decryptCipher = Cipher.getInstance("AES/CBC/PKCS5Padding"); | ||
| decryptCipher.init(Cipher.DECRYPT_MODE, skeySpec, ivector); | ||
| if (decryptCipher != null) { |
There was a problem hiding this comment.
Why not declare, initialize and instantiate the decryptCipher here itself? Why is a static member to this class needed when the Cipher.getInstance(...) itself is static?
There was a problem hiding this comment.
Looks like the code earlier was doing what I am talking about, above. Why is this change for declaring decryptCipher as a static class member needed?
There was a problem hiding this comment.
There is no reason for the decryptCipher variable to be static, I will change this. However, I believe it should be done once in the constructor as there isn't a point to get an instance for every decryptData(byte,byte,int,int) call.
There was a problem hiding this comment.
The issue I see with having this in the constructor is that you are replicating the behavior of Cipher.getInstance() which is static itself and adding this additional layer of caching and making assumption about the API that it would be safe to use the same object when the API itself doesn't make any such guarantees in the documentation.
A static is good if you want to cache the object at your end and it is safe to do so, but since Cipher.getInstance(...) is static itself, it should decide whether it should give a new object everytime or return a cached object.
In case the behavior of Cipher.getInstance() changes for any reason across JDK versions, then this driver will have to comb through this caching. Such bugs are hard to detect. Such layers of caching of objects returned from static call is not a good practice.
There was a problem hiding this comment.
Agreed, I will revert the changes.
|
What is the cause of degradation? The logger traversing the stack or lack of initialization of cipher? |
Codecov Report
@@ Coverage Diff @@
## dev #773 +/- ##
============================================
- Coverage 48.17% 48.14% -0.03%
- Complexity 2776 2777 +1
============================================
Files 116 116
Lines 27854 27848 -6
Branches 4636 4636
============================================
- Hits 13418 13408 -10
- Misses 12215 12219 +4
Partials 2221 2221
Continue to review full report at Codecov.
|
| try { | ||
| decryptCipher = Cipher.getInstance("AES/CBC/PKCS5Padding"); | ||
| } catch (NoSuchAlgorithmException | NoSuchPaddingException e) { | ||
| // not possible to get here |
There was a problem hiding this comment.
From the comment, it sounds like these exceptions is not expected to ever happen. How come we are try-catching and then swallowing these exceptions then? Shouldn't we let these exceptions get thrown if something unexpected occurs?
There was a problem hiding this comment.
These exceptions must be logged. I can see the source of OpenJDK where this exception can be thrown. In case the Cipher is not found in the security providers, then an exception can definitely surface.
|
Uploading jars. These jars contain the final changes that will be going into the driver. |
rene-ye
changed the title
rene-ye
changed the title
Fixed an issue where the logger was needlessly traversing the stack to obtain method name.