Please Review Test Fix of Bug 7177045

Brad Wetmore bradford.wetmore at oracle.com
Thu Jun 28 17:30:34 PDT 2012


Dan, congrats on assembling and posting your first webrev.  Besides the 
big picture things, since you are new, I'll also be looking for minor 
things that you may or may not know yet.

On 6/28/2012 1:49 PM, Dan Xu wrote:
> Security code reviewers,
>
> I have fixed a security test failure and posted my changes at
> http://cr.openjdk.java.net/~dxu/7177045/. Please help review it. Thanks!

Minor nit:  line 38 has a space at the end of the line.  Current jstyle 
guidelines state no indention with tabs and no whitespace at the end of 
the lines.

Lines 61/89:  memroy->memory

Just wondering why you chose a Deque instead of a simpler LinkedList?

Suggest more liberal use of comments, either in the method's comments or 
inline.  Good to explain your assumptions/approach in case things aren't 
obvious.  For example, why do you backoff 3MB after allocating available 
memory?  And at line 134:  the operation could either time out or threw 
an exception.  Nice to make that clear.

dummyData could be a local variable.

Line 64/113:  consider using the JDK 7 diamond <> operator on your generics.

Line 114:  consider adding a @overrides annotation on the call() method.

Line 139:  I'm being paranoid here, but shutdownNow doesn't guarantee 
threads will be stopped.  If we actually got into a situation where 
there was a timeout, executor.shutdownNow() *may* never return.  One 
reason is it might be hanging somewhere waiting for memory.  I would 
suggest as part of your finally block, you dequeue all the memory in 
dummyData, call System.gc(), then run executor.shutdownNow(). JTREG will 
timeout after two minutes, but if we can proactively help the situation, 
we might as well.

Otherwise, looks good.  We'll wait to see if anyone has other thoughts, 
and if not, we'll push when you're back from vacation.

Brad



More information about the security-dev mailing list