Hello Valeri, Thank you for review. Please, have a look at updated patch at : https://cr.openjdk.java.net/~abakhtin/8241960/webrev.v1/
I’ve fixed your findings and renamed test class to ThreadSafetyTest. Jtreg already has ThreadSafeTest class for java.text.Normalizer functionality. Thank you Alexey On 3 Apr 2020, at 02:51, Valerie Peng <valerie.p...@oracle.com<mailto:valerie.p...@oracle.com>> wrote: Hi Alexey, In general looks pretty good, just some comments on the regression test: - line 28: The duration value 10 may be lowered to shorten the execution time. On a Linux machine, with threadFactor=5, each digest algo takes about (2xduration+2) sec and overall takes ~283sec. Cutting the duration value drastically to 2, I still can reproduce the bug. Maybe 4 would be a better default value considering the execution time. - line 43: add "SHA-512/224", "SHA-512/256" to the ALGORITHM_ARRAY? - line 50: instead of hardcoding 5 here, why not just use threadsFactor (assigned with default value 5 on line 48)? - line 52-55: I think you meant to use 5 (instead of the value 180) as the default min duration on line 52. Then, use duration variable instead of the hardcoded 5 on line54. For testing purpose, why not just use the supplied value? You already have default values if none are supplied. - Consider iterating through existing providers and remove isSHA3supported() method. This way all providers which supports the digest algorithm are tested and no provider-specific knowledge is needed. For example: Provider[] provs = Security.getProviders(); for (Provider p : provs) { System.out.println("Testing provider " + p.getName() + "..."); for (String alg: ALGORITHM_ARRAY) { try { MessageDigest md = MessageDigest.getInstance(alg, p); testThreadSafe(md, input, nTasks, duration, false); .... - line 76: missing space char between } and catch. Thanks, Valerie
