Request for review (S) 7009828: Fix for 6938627 breaks visualvm monitoring when -Djava.io.tmpdir is defined
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Jan 12 15:59:09 PST 2011
On 1/12/2011 4:52 PM, Kelly O'Hair wrote:
> 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.
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)... :-)
> 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
>> 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 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.
>>> 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