code review for attach on demand (AOD) fix for Windows (7028668)

[Daniel D. Daugherty]

Alan,

Thanks for the review!

Replies below...

On 3/18/2011 3:14 AM, Alan Bateman wrote:
> Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I have minor fixes to the Attach On Demand (AOD) Windows
>> specific code that I'd like to get into T&L snapshot for
>> OpenJDK7-B136 (next week):
>>
>>    http://cr.openjdk.java.net/~dcubed/7028668-webrev/0/
>>
>> I'm adding more info to the default detail message for an
>> OpenProcess() failure. I'm also getting a process handle
>> in a different way when a Java process tries to attach to
>> itself. If the new technique fails, then we fall back to
>> the original OpenProcess() code.
>>
>> Thanks, in advance, for any comments.
>>
>> Dan
> Can you elaborate more on why you are using DuplicateHandle when the 
> target is the current process?

Yes, GetCurrentProcess() returns a "special" value to indicate
the current process. Currently that value is -1. Microsoft's
docs indicate that GetCurrentProcess() should be used instead
of hard coding the special value.

We have to use DuplicateHandle() on the special value so that
the process handle we return can be used. The special value
of -1 causes other parts of our attach system to be unhappy.
Calling DuplicateHandle() is also recommended by the Microsoft
docs if you're planning to pass around the returned handle.


> It doesn't really make practical sense to attach to the current 
> process but I realize there may be tests that do that.

The first attach test in the nsk.jvmti subsuite is of the "warm
and breathing" variety. It's a simple single process test that
makes sure that the attach mechanism works. No multi process
coordination. Just plain and simple. And every now and then, it
catches this OpenProcess() failure which I believe takes down
the rest of the attach tests. So that makes it a useful test :-)


> I suspect you may be chasing a resource issue.

I'm not convinced yet. Since I haven't been able to reproduce
the OpenProcess() failure mode myself, I hesitate to guess.
I will take another look at the 2011.02.18 failure set to see
if I see other signs of resource problems.


> If OpenProcess fails then ThrowIOExceptionWithLastError will attempt 
> translate the error into a system message. I did a few tests and it 
> does translate the errors as intended.

Yup. When I was testing these changes, I induced some failure modes
and the system dutifully provided me with an exception that was more
descriptive than the "default detail" one.


> In any case, putting the error code into the fallback system to help 
> diagnose this seems reasonable to me.

Thanks!


> Agree with Kelly on the sprintf.

Yup. We'll get there.

Thanks again for the review.

Dan


>
> -Alan.
>
>