Request for review (S) 7009828: Fix for 6938627 breaks visualvm monitoring when -Djava.io.tmpdir is defined
coleen.phillimore at oracle.com
Wed Jan 12 11:19:35 PST 2011
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
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
> 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
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.
> 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
More information about the hotspot-runtime-dev