Hi Weijun!
A few comments.
1)
Since PRNG is Serializable, shouldn't `ctxt` be declared transient?
2)
The documentation of Cleaner [1] suggests not to use lambdas for
cleaning actions, as it may accidentally capture a reference to the
object being cleaned.
[1]
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ref/Cleaner.html
3)
Do you know if it is true that the context handle might be used
concurrently?
I couldn't find in the MSDN online documentation specific promises
(there is a link to "Threading Issues with Cryptographic Service
Providers" article, but the page is empty).
Would it make sense to maintain the only context handler in a static
WeakReference<Context> field?
So that it is reused with all instances of PRNG and is cleared when the
last instance of PRNG is gone.
Not sure if it is really necessary, as a normal application wouldn't
probably need several instances of PRNG, but could prevent
out-of-available-handles trouble in some extreme cases.
With kind regards,
Ivan.
On 11/26/18 5:33 PM, Weijun Wang wrote:
Ping again
On Nov 20, 2018, at 5:33 PM, Weijun Wang <weijun.w...@oracle.com> wrote:
Webrev updated at
https://cr.openjdk.java.net/~weijun/8210476/webrev.01/
The only change is that there is a single Cleaner now for the whole PRNG class.
Otherwise, each will maintain its own thread.
Thanks
Max
On Nov 11, 2018, at 11:30 PM, Weijun Wang <weijun.w...@oracle.com> wrote:
Please take a review at
https://cr.openjdk.java.net/~weijun/8210476/webrev.00/
Before this fix, every PRNG::nextBytes calls all of CryptAcquireContext,
CryptGenRandom, and CryptReleaseContext. Now, CryptAcquireContext is called
once in PRNG::new, and CryptReleaseContext is called by a Cleaner, and
nextBytes only calls CryptGenRandom.
I haven't read about thread-safety in any MS document, the current Windows-PRNG
service is marked ThreadSafe=true (in SunMSCAPI.java). If we cannot be really
sure, we can change it to false.
I've downloaded nearly 1000 Mach5 runs of this test, the enhancement is so good
that I adjusted the test to be stricter.
Before After
------ -----
Count 897 74
Min 0.38 0.008
Ave 0.97 0.011
Max 5.81 0.021
Please advise me if the following usage of Cleaner is correct because I really
haven't observed the releaseContext method being called.
+ Cleaner.create().register(this,
+ () -> releaseContext(ctxt));
Thanks
Max
--
With kind regards,
Ivan Gerasimov
