RFR: 8148188: Enhance the security libraries to record events of interest
sean.coffey at oracle.com
Tue Jul 10 12:50:42 UTC 2018
After some trial edits, I'm not so sure if moving the event & logger
commit code into the class where it's used works too well after all.
In the code you suggested, there's an assumption that calls such as
EventHelper.certificateChain(..) are low cost. While that might be the
case here, it's something we can't always assume. It's called once for
the JFR operation and once for the Logger operation. That pattern
multiplies itself further in other Events. i.e. set up and assign the
variables for a JFR event without knowing whether they'll be needed
again for the Logger call. We could work around it by setting up some
local variables and re-using them in the Logger code but then, we're
back to where we were. The current EventHelper design eliminates this
problem and also helps to abstract the recording/logging functionality
away from the functionality of the class itself.
It also becomes a problem where we record events in multiple different
classes (e.g. SecurityPropertyEvent). While we could leave the code in
EventHelper for this case, we then have a mixed design pattern.
Are you ok with leaving as is for now? A future wish might be one where
JFR would handle Logger via its own framework/API in a future JDK release.
I've removed the variable names using underscore. Also optimized some
variable assignments in X509Impl.commitEvent(..)
On 09/07/2018 18:01, Seán Coffey wrote:
> Thanks for reviewing. Comments inline..
> On 09/07/18 17:21, Erik Gahlin wrote:
>> Thanks Sean.
>> Some feedback on the code in the security libraries.
>> - We should use camel case naming convention for variables (not
> Sure. I see two offending variable names which I'll fix up.
>> - Looking at sun/security/ssl/Finished.java,
>> I wonder if it wouldn't be less code and more easy to read, if we
>> would commit the event in a local private function and then use the
>> EventHelper class for common functionality.
> I'm fine with your suggested approach. I figured that tucking the
> recording/logging logic away in a helper class also had benefits but
> I'll re-factor to your suggested style unless I hear objections.
More information about the core-libs-dev