Please Review Test Fix of Bug 7177045

[Dan Xu]

Hi Brad,

Thanks for your good suggestions. I have fixed most of them and 
re-uploaded my changes at http://cr.openjdk.java.net/~dxu/7177045.01/.

The reason that I chose ArrayDeque instead of LinkedList is that 
ArrayDequeseems have better performance. According to the java doc, 
"most ArrayDeque operations run in amortized constant time" and "this 
class is likely to be faster then LinkedList when used as a queue." It 
is also very easy to remove last elements to back off memory allocation.

In addition, I did not switch to diamond operator. Because old Jdk 
bundles, say jdk 1.7.0-ea-b23 and jdk 1.7.0-ea-b29 used in my testing, 
failed to compile diamond operator. Here are the compilation error messages,

    TestProviderLeak.java:62: illegal start of type
             Deque<byte []> data = new ArrayDeque<>();
                                                  ^
    1 error


I guess those jdk might be too early to adopt the diamond operator 
changes. I am not sure whether we still take these old jdk bundles into 
consideration here. Thanks!

-Dan

On 06/28/2012 05:30 PM, Brad Wetmore wrote:
> 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


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20120705/c27695f9/attachment.htm>