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

[Daniel D. Daugherty]

On 1/12/2011 4:52 PM, Kelly O'Hair wrote:
> Ok, I'll back away, .... carefully. ;^)

Thanks!


> 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.

I'm pretty sure we've "proven" that for some degree of proof...


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

You never know. Someone else will probably come along with some
Grand Temp File Unification theory (GTFU)... :-)

Dan


>
> -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
>>>>
>>>
>>
>