Request for review (S) 7009828: Fix for 6938627 breaks visualvm monitoring when -Djava.io.tmpdir is defined

[Kelly O'Hair]

Ok, I'll back away, .... carefully. ;^)

As long as any use of the temp directory in hotspot is careful about  
what it creates in that area,
it's limited to hotspot, it's the way it was before, etc. etc.

Sorry for beating the dead horse, it is dead right? ;^)

-kto

On Jan 12, 2011, at 11:19 AM, Coleen Phillimore wrote:

> I put (S) in the title but it meant small, not simple.   More below.
>
> On 1/12/2011 1:29 PM, Kelly O'Hair wrote:
>> I'm a little concerned about this change.
>>
>> On windows, this relies on GetTempPath, which seems to be  
>> influenced by the environment
>> variables TEMP or TMP. So first question is, what happens if TEMP  
>> or TMP are both undefined
>> or they refer to a directory that does not exist?
>> I recently discovered that the VS2010 compiler just doesn't work if  
>> TMP or TEMP are not defined properly,
>> and with very little hints as to why.  There is no guarantee that  
>> it doesn't just return 0 or a path that does
>> not exist. It can fail and then you have to call GetLastError to  
>> find out why, most people don't. :^(
>
> I'm not surprised VS2010 compilers don't work without TEMP or TMP  
> defined.  I don't think a lot of things would work on windows.  I'll  
> test it out on my machine and see what sort of error we get, but we  
> really only use get_temp_directory() for a couple things that have  
> backoffs if there's an error opening the file.  Also if GetTempPath  
> returns null we return "", which should use the current working  
> directory (except one case in hs_err, I added a path separator,  
> which I should only conditionally add).
>
>>
>> On Solaris/Linux, this just hardwires /tmp, which of course should  
>> always exist, but it is not influenced
>> by any environment variables like TMPDIR. I'm not sure what the  
>> rules should be about obeying the value
>> of TMPDIR. And as I recall, TMPDIR is supposed to be a system wide  
>> temp area replacement for /tmp
>> on Solaris at least. Do we care about TMPDIR? Do we want to allow  
>> people to re-direct the temp directory
>> location?
> I think TMPDIR can be a user environment variable too, and we don't  
> want user environment variables.  Systems without /tmp or define tmp  
> to be something else can be problematic, which is why we had changed  
> it to use java.io.tmpdir.  But this broke a lot of other things.   
> When we port to said systems with different tmps we can fix the code  
> for these then with some #ifdefs or whatever.
>
>>
>> Should the selection of the VM temp directory be influenced by  
>> environment variables?
>> Or is hardwiring to /tmp a good idea?
>> Should the temp directory be verified as far as existence and  
>> permissions?
>> The java.io.tmpdir is /var/tmp on Linux and /tmp on Solaris as I  
>> recall, does that matter?
> It does actually matter  because /tmp is apparently faster on  
> solaris and using java.io.tmpdir as /var/tmp could slow down  
> perfdata.  There's a lot of discussion in these bugs (the one I  
> pointed to and also the see-also bugs).  The code that tries to  
> create files based on non-existent temp directories will fail.  We  
> do have failure messages for those in all cases.
>
>>
>> I know this seems like a simple change, but I'm not so sure it is  
>> that simple.
> No, I agree it's not a simple matter, which is why it took a while  
> to decide to revert the change we made.
>>
>> When I run tests I often re-define java.io.tmpdir so that I don't  
>> collide with anyone else running tests
>> on the same system. Now I can't. You can argue the testcase is bad  
>> if it doesn't insure it creates unique
>> filenames in /tmp, so I won't use that as an argument to not change  
>> things. But I wonder how sloppy
>> we are in creating files in /tmp, are all uses making sure that  
>> they are unique and not used by anyone else?
>
> When the VM uses /tmp or %TEMP% (or %TMP%) it puts the process id in  
> the file because it's assumed to be in a  common place with other  
> java processes running.  The property java.io.tmpdir allows users to  
> write Java code that doesn't have to make that assumption, and that  
> is why this is breaking.  Apparently  (according to the bug report)  
> running Tomcat with java.io.tmpdir is necessary because of the  
> tomcat temporary files.  The VM's temporary files are going to be  
> different.  They need to be per-system.
>
> I don't really know if hardcoding /tmp is a good idea or not but  
> that's what we've had before and changing it is going to cause  
> problems for people that they will (have already) report as  
> regressions.   This change changes it back to what we had.
>
> thanks,
> Coleen
>
>
>>
>> -kto
>>
>> On Jan 12, 2011, at 9:38 AM, Coleen Phillimore wrote:
>>
>>> Summary: Change get_temp_directory() back to /tmp and %TEMP% like  
>>> it always was and where the tools expect it to be.
>>>
>>> There is more information in the bug, but essentially changing  
>>> get_temp_directory() to use java.io.tmpdir breaks a lot of things.
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/7009828/
>>> bug link at http://bugs.sun.com/view_bug.do?bug_id=7009828
>>>
>>> Thanks,
>>> Coleen
>>>
>>
>