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

[Coleen Phillimore]

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