Re: A Bug about the JVM Attach mechanism

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: A Bug about the JVM Attach mechanism

Bernd Eckenfels-4
The simple solution is probably not delete those files. :)

If this is a long known bug, did you actually open a bug report with any of the OpenJDK contributor organizations and what’s the number?

Gruss
Bernd


--
http://bernd.eckenfels.net
 

Von: serviceability-dev <[hidden email]> im Auftrag von nijiaben <[hidden email]>
Gesendet: Mittwoch, Juni 19, 2019 7:12 PM
An: serviceability-dev; jdk8u-dev; hotspot-runtime-dev
Betreff: A Bug about the JVM Attach mechanism
 
Hi, All,

There is a bug that has been discovered for a long time, but it has not been fixed. 

We usually use some shell scripts to clean up the /tmp directory. 
When we have executed a similar jstack command based on the attach mechanism, 
it will generate a .java_pidXXX socket file in the /tmp directory. 
Once this file is clean up, and then will not be attached anymore.

The main reason is that once the Attach Listener thread is created, 
the file of  “/tmp/.java_pidXXX” will be created and the markup initialization is complete. 
If .java_pidXXX is cleaned up, it will not be created anymore and cannot be attached.

bool AttachListener::is_init_trigger() {
if (init_at_startup() || is_initialized()) { // initialized at startup or already initialized
return false;
}
char fn[PATH_MAX+1];
sprintf(fn, ".attach_pid%d", os::current_process_id());
int ret;
struct stat64 st;
RESTARTABLE(::stat64(fn, &st), ret);
if (ret == -1) {
snprintf(fn, sizeof(fn), "%s/.attach_pid%d",
os::get_temp_directory(), os::current_process_id());
RESTARTABLE(::stat64(fn, &st), ret);
}
if (ret == 0) {
if (st.st_uid == geteuid()) {
init(); // will create Attach Listener Thread
return true;
}
}
return false;
}

This bug has a big impact on troubleshooting some problems. 
Can it be fixed as soon as possible?

Thanks,

nijiaben @ PerfMa

Reply | Threaded
Open this post in threaded view
|

Re: A Bug about the JVM Attach mechanism

nijiaben
Hi,

> Can you provide a bug number for this?

I'm sorry, I made you misunderstand what I meant.
I mean, I knew this problem before, but I didn't submit it to the JDK Bug System.
 
> Once we have attached and created the attach thread we don't need to 
> perform that initialization handshake because the attach thread exists 
> and will process any subsequent commands.

I know this mechanism, can we provide means of recovery to avoid unavailability caused by accidental deletion?

Thanks,

nijiaben @ PerfMa

------------------ Original ------------------
Date:  Thu, Jun 20, 2019 11:36 AM
To:  "nijiaben"<[hidden email]>; "serviceability-dev"<[hidden email]>; "jdk8u-dev"<[hidden email]>; "hotspot-runtime-dev"<[hidden email]>;
Subject:  Re: A Bug about the JVM Attach mechanism
 
Hi,

On 19/06/2019 2:26 am, nijiaben wrote:
> Hi, All,
>
> There is a bug that has been discovered for a long time, but it has not
> been fixed.

Can you provide a bug number for this?

> We usually use some shell scripts to clean up the /tmp directory.
> When we have executed a similar jstack command based on the attach
> mechanism,
> it will generate a .java_pidXXX socket file in the /tmp directory.
> Once this file is clean up, and then will not be attached anymore.
>
> The main reason is that once the Attach Listener thread is created,
> the file of  “/tmp/.java_pidXXX” will be created and the markup
> initialization is complete.
> If .java_pidXXX is cleaned up, it will not be created anymore and cannot
> be attached.

Once we have attached and created the attach thread we don't need to
perform that initialization handshake because the attach thread exists
and will process any subsequent commands.

David
-----


>
> bool AttachListener::is_init_trigger() {
> if (init_at_startup() || is_initialized()) { // initialized at startup
> or already initialized
> return false;
> }
> char fn[PATH_MAX+1];
> sprintf(fn, ".attach_pid%d", os::current_process_id());
> int ret;
> struct stat64 st;
> RESTARTABLE(::stat64(fn, &st), ret);
> if (ret == -1) {
> snprintf(fn, sizeof(fn), "%s/.attach_pid%d",
> os::get_temp_directory(), os::current_process_id());
> RESTARTABLE(::stat64(fn, &st), ret);
> }
> if (ret == 0) {
> if (st.st_uid == geteuid()) {
> init(); // will create Attach Listener Thread
> return true;
> }
> }
> return false;
> }
>
> This bug has a big impact on troubleshooting some problems.
> Can it be fixed as soon as possible?
>
> Thanks,
>
> nijiaben @ PerfMa
>
Reply | Threaded
Open this post in threaded view
|

Re: A Bug about the JVM Attach mechanism

Krystal Mok
(Speaking on behalf of my own experience) Yes Alan, you're right on. I'm fairly sure that's the symptom that Nijiaben is asking about here.

- Kris

On Wed, Jun 19, 2019 at 11:54 PM Alan Bateman <[hidden email]> wrote:
On 20/06/2019 05:10, nijiaben wrote:
> :
> I know this mechanism, can we provide means of recovery to avoid unavailability caused by accidental deletion?
>
Are you concerned about tmpreaper or cron jobs that periodically cleanup
/tmp? There may indeed be an issue for applications that run for weeks
or months. If someone is using jmap, jcmd or other tools using the
attach API then it will trigger the attach listener to start. When they
come back in a few weeks then the .java_pid<pid> file may have been
removed so they cannot attach. Is this what what you are pointing out?

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: A Bug about the JVM Attach mechanism

Leonid Mesnik
In reply to this post by nijiaben
I think it is

The jcmd failed to attach if /tmp/.java_pid* is removed. It doesn't find file which is used as listener socket  and send signal to start attach listener. (It suppose that listener is not started yet). While target VM expect that listener is started already and just dump stack trace into stdout.


See code

The listener is initialized only once when thread is started and still left "initialized" even socket is removed.

The only workaround is not to remove this file. 

Leonid

On Jun 19, 2019, at 9:12 PM, David Holmes <[hidden email]> wrote:

On 19/06/2019 2:10 pm, nijiaben wrote:
Hi,
> Can you provide a bug number for this?
I'm sorry, I made you misunderstand what I meant.
I mean, I knew this problem before, but I didn't submit it to the JDK Bug System.

Okay.

> Once we have attached and created the attach thread we don't need to
> perform that initialization handshake because the attach thread exists
> and will process any subsequent commands.
I know this mechanism, can we provide means of recovery to avoid unavailability caused by accidental deletion?

Sorry I'm still not clear how deletion causes any problem after the attach thread has been started ??

David
-----

Thanks,
nijiaben @ PerfMa
------------------ Original ------------------
*From: * "David Holmes"<[hidden email]>;
*Date: * Thu, Jun 20, 2019 11:36 AM
*To: * "nijiaben"<[hidden email]>; "serviceability-dev"<[hidden email]>; "jdk8u-dev"<[hidden email]>; "hotspot-runtime-dev"<[hidden email]>;
*Subject: * Re: A Bug about the JVM Attach mechanism
Hi,
On 19/06/2019 2:26 am, nijiaben wrote:
> Hi, All,
>
> There is a bug that has been discovered for a long time, but it has not
> been fixed.
Can you provide a bug number for this?
> We usually use some shell scripts to clean up the /tmp directory.
> When we have executed a similar jstack command based on the attach
> mechanism,
> it will generate a .java_pidXXX socket file in the /tmp directory.
> Once this file is clean up, and then will not be attached anymore.
>
> The main reason is that once the Attach Listener thread is created,
> the file of  “/tmp/.java_pidXXX” will be created and the markup
> initialization is complete.
> If .java_pidXXX is cleaned up, it will not be created anymore and cannot
> be attached.
Once we have attached and created the attach thread we don't need to
perform that initialization handshake because the attach thread exists
and will process any subsequent commands.
David
-----
>
> bool AttachListener::is_init_trigger() {
> if (init_at_startup() || is_initialized()) { // initialized at startup
> or already initialized
> return false;
> }
> char fn[PATH_MAX+1];
> sprintf(fn, ".attach_pid%d", os::current_process_id());
> int ret;
> struct stat64 st;
> RESTARTABLE(::stat64(fn, &st), ret);
> if (ret == -1) {
> snprintf(fn, sizeof(fn), "%s/.attach_pid%d",
> os::get_temp_directory(), os::current_process_id());
> RESTARTABLE(::stat64(fn, &st), ret);
> }
> if (ret == 0) {
> if (st.st_uid == geteuid()) {
> init(); // will create Attach Listener Thread
> return true;
> }
> }
> return false;
> }
>
> This bug has a big impact on troubleshooting some problems.
> Can it be fixed as soon as possible?
>
> Thanks,
>
> nijiaben @ PerfMa
>

Reply | Threaded
Open this post in threaded view
|

Re: A Bug about the JVM Attach mechanism

Yasumasa Suenaga-4
In reply to this post by nijiaben
Hi,

Can we fix this issue like this webrev?
   http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal/

I could reproduce this issue with ConcAttachTest.java in it
on my laptop.
(Fedora 30 x64 on Hyper-V guest: Core i3-8145U x 2vcpu)

If we need to fix based on current implementation, I think
we need to use atomic operation.
But if we can refactor around here, we might be able to another approach
- e.g. using monitors/mutexes


Thanks,

Yasumasa


On 2019/06/21 5:49, David Holmes wrote:

> Sorry it took me a while to understand the specifics of the problem. :)
>
> David
>
> On 20/06/2019 3:37 am, nijiaben wrote:
>> Yes Alan, I mean this
>> ------------------?Original?------------------
>> *From: *?"Alan Bateman"<Alan.Bateman at oracle.com>;
>> *Date: *?Thu, Jun 20, 2019 02:54 PM
>> *To: *?"nijiaben"<nijiaben at perfma.com>; "David
>> Holmes"<david.holmes at oracle.com>;
>> "serviceability-dev"<serviceability-dev at openjdk.java.net>;
>> "jdk8u-dev"<jdk8u-dev at openjdk.java.net>;
>> "hotspot-runtime-dev"<hotspot-runtime-dev at openjdk.java.net>;
>> *Subject: *?Re: A Bug about the JVM Attach mechanism
>> On 20/06/2019 05:10, nijiaben wrote:
>>   > :
>>   > I know this mechanism, can we provide means of recovery to avoid
>> unavailability caused by accidental deletion?
>>   >
>> Are you concerned about tmpreaper or cron jobs that periodically cleanup
>> /tmp? There may indeed be an issue for applications that run for weeks
>> or months. If someone is using jmap, jcmd or other tools using the
>> attach API then it will trigger the attach listener to start. When they
>> come back in a few weeks then the .java_pid<pid> file may have been
>> removed so they cannot attach. Is this what what you are pointing out?
>>
>> -Alan
Reply | Threaded
Open this post in threaded view
|

Re: A Bug about the JVM Attach mechanism

David Holmes
Hi Yasumasa,

On 21/06/2019 5:12 am, Yasumasa Suenaga wrote:
> Hi,
>
> Can we fix this issue like this webrev?
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal/

I'm not clear how this addresses the issue with deleting the file? The
file still has to exist IIUC for the mechanism to work.

This seems more suited to fixing the "multiple attach threads" problem.

David

> I could reproduce this issue with ConcAttachTest.java in it
> on my laptop.
> (Fedora 30 x64 on Hyper-V guest: Core i3-8145U x 2vcpu)
>
> If we need to fix based on current implementation, I think
> we need to use atomic operation.
> But if we can refactor around here, we might be able to another approach
> - e.g. using monitors/mutexes
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2019/06/21 5:49, David Holmes wrote:
>> Sorry it took me a while to understand the specifics of the problem. :)
>>
>> David
>>
>> On 20/06/2019 3:37 am, nijiaben wrote:
>>> Yes Alan, I mean this
>>> ------------------?Original?------------------
>>> *From: *?"Alan Bateman"<Alan.Bateman at oracle.com>;
>>> *Date: *?Thu, Jun 20, 2019 02:54 PM
>>> *To: *?"nijiaben"<nijiaben at perfma.com>; "David
>>> Holmes"<david.holmes at oracle.com>;
>>> "serviceability-dev"<serviceability-dev at openjdk.java.net>;
>>> "jdk8u-dev"<jdk8u-dev at openjdk.java.net>;
>>> "hotspot-runtime-dev"<hotspot-runtime-dev at openjdk.java.net>;
>>> *Subject: *?Re: A Bug about the JVM Attach mechanism
>>> On 20/06/2019 05:10, nijiaben wrote:
>>>   > :
>>>   > I know this mechanism, can we provide means of recovery to avoid
>>> unavailability caused by accidental deletion?
>>>   >
>>> Are you concerned about tmpreaper or cron jobs that periodically cleanup
>>> /tmp? There may indeed be an issue for applications that run for weeks
>>> or months. If someone is using jmap, jcmd or other tools using the
>>> attach API then it will trigger the attach listener to start. When they
>>> come back in a few weeks then the .java_pid<pid> file may have been
>>> removed so they cannot attach. Is this what what you are pointing out?
>>>
>>> -Alan
Reply | Threaded
Open this post in threaded view
|

Re: A Bug about the JVM Attach mechanism

Yasumasa Suenaga-4
On 2019/06/21 22:51, David Holmes wrote:

> Hi Yasumasa,
>
> On 21/06/2019 5:12 am, Yasumasa Suenaga wrote:
>> Hi,
>>
>> Can we fix this issue like this webrev?
>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal/
>
> I'm not clear how this addresses the issue with deleting the file? The
> file still has to exist IIUC for the mechanism to work.
>
> This seems more suited to fixing the "multiple attach threads" problem.

Yes, my proposal focuses to "multiple attach threads".
I shared my patch because it might help the work for this issue.

This patch would guard multithread-issue in Attach Listener.
So unix domain socket file will create just once.


Yasumasa


> David
>
>> I could reproduce this issue with ConcAttachTest.java in it
>> on my laptop.
>> (Fedora 30 x64 on Hyper-V guest: Core i3-8145U x 2vcpu)
>>
>> If we need to fix based on current implementation, I think
>> we need to use atomic operation.
>> But if we can refactor around here, we might be able to another
>> approach - e.g. using monitors/mutexes
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> On 2019/06/21 5:49, David Holmes wrote:
>>> Sorry it took me a while to understand the specifics of the problem. :)
>>>
>>> David
>>>
>>> On 20/06/2019 3:37 am, nijiaben wrote:
>>>> Yes Alan, I mean this
>>>> ------------------?Original?------------------
>>>> *From: *?"Alan Bateman"<Alan.Bateman at oracle.com>;
>>>> *Date: *?Thu, Jun 20, 2019 02:54 PM
>>>> *To: *?"nijiaben"<nijiaben at perfma.com>; "David
>>>> Holmes"<david.holmes at oracle.com>;
>>>> "serviceability-dev"<serviceability-dev at openjdk.java.net>;
>>>> "jdk8u-dev"<jdk8u-dev at openjdk.java.net>;
>>>> "hotspot-runtime-dev"<hotspot-runtime-dev at openjdk.java.net>;
>>>> *Subject: *?Re: A Bug about the JVM Attach mechanism
>>>> On 20/06/2019 05:10, nijiaben wrote:
>>>>   > :
>>>>   > I know this mechanism, can we provide means of recovery to avoid
>>>> unavailability caused by accidental deletion?
>>>>   >
>>>> Are you concerned about tmpreaper or cron jobs that periodically
>>>> cleanup
>>>> /tmp? There may indeed be an issue for applications that run for weeks
>>>> or months. If someone is using jmap, jcmd or other tools using the
>>>> attach API then it will trigger the attach listener to start. When they
>>>> come back in a few weeks then the .java_pid<pid> file may have been
>>>> removed so they cannot attach. Is this what what you are pointing out?
>>>>
>>>> -Alan
Reply | Threaded
Open this post in threaded view
|

Re: A Bug about the JVM Attach mechanism

David Holmes
In reply to this post by nijiaben
Hi Peter,

On 21/06/2019 7:55 am, Peter Levart wrote:

> As far as I know, cron jobs that cleanup /tmp typically remove files
> that have not been modified for a while.
>
> On Fedora for example, there is a systemd timer that triggers once per
> day and executes systemd-tmpfiles which manages volatile and temporary
> files and directories. The configuration for /tmp is the following:
>
> # Clear tmp directories separately, to make them easier to override
> q /tmp 1777 root root 10d
> q /var/tmp 1777 root root 30d
>
> The age field (10 days for /tmp) has the following meaning:
>
>         The age of a file system entry is determined from its last
> modification timestamp (mtime), its last access timestamp (atime), and
> (except for directories) its last status change
>         timestamp (ctime). Any of these three (or two) values will
> prevent cleanup if it is more recent than the current time minus the age
> field.
>
> So the solution could be for attach thread (if it is already started) to
> update mtime or ctime of the .java_pid<pid> socket file periodically so
> cleanup job would leave it alone.
>
> What do you think?

I'm not keen on having the attach listener thread periodically wakeup
just to do this. Can we not change permissions to protect the file form
external deletion and ensure the VM cleans it up itself?

David

> Regards, Peter
>
>
> On 6/20/19 10:49 PM, David Holmes wrote:
>> Sorry it took me a while to understand the specifics of the problem. :)
>>
>> David
>>
>> On 20/06/2019 3:37 am, nijiaben wrote:
>>> Yes Alan, I mean this
>>> ------------------ Original ------------------
>>> *From: * "Alan Bateman"<[hidden email]>;
>>> *Date: * Thu, Jun 20, 2019 02:54 PM
>>> *To: * "nijiaben"<[hidden email]>; "David
>>> Holmes"<[hidden email]>;
>>> "serviceability-dev"<[hidden email]>;
>>> "jdk8u-dev"<[hidden email]>;
>>> "hotspot-runtime-dev"<[hidden email]>;
>>> *Subject: * Re: A Bug about the JVM Attach mechanism
>>> On 20/06/2019 05:10, nijiaben wrote:
>>>  > :
>>>  > I know this mechanism, can we provide means of recovery to avoid
>>> unavailability caused by accidental deletion?
>>>  >
>>> Are you concerned about tmpreaper or cron jobs that periodically cleanup
>>> /tmp? There may indeed be an issue for applications that run for weeks
>>> or months. If someone is using jmap, jcmd or other tools using the
>>> attach API then it will trigger the attach listener to start. When they
>>> come back in a few weeks then the .java_pid<pid> file may have been
>>> removed so they cannot attach. Is this what what you are pointing out?
>>>
>>> -Alan
>
Reply | Threaded
Open this post in threaded view
|

Re: A Bug about the JVM Attach mechanism

Peter Levart
Hi David,

On 6/21/19 5:57 PM, David Holmes wrote:

> Hi Peter,
>
> On 21/06/2019 7:55 am, Peter Levart wrote:
>> As far as I know, cron jobs that cleanup /tmp typically remove files
>> that have not been modified for a while.
>>
>> On Fedora for example, there is a systemd timer that triggers once
>> per day and executes systemd-tmpfiles which manages volatile and
>> temporary files and directories. The configuration for /tmp is the
>> following:
>>
>> # Clear tmp directories separately, to make them easier to override
>> q /tmp 1777 root root 10d
>> q /var/tmp 1777 root root 30d
>>
>> The age field (10 days for /tmp) has the following meaning:
>>
>>         The age of a file system entry is determined from its last
>> modification timestamp (mtime), its last access timestamp (atime),
>> and (except for directories) its last status change
>>         timestamp (ctime). Any of these three (or two) values will
>> prevent cleanup if it is more recent than the current time minus the
>> age field.
>>
>> So the solution could be for attach thread (if it is already started)
>> to update mtime or ctime of the .java_pid<pid> socket file
>> periodically so cleanup job would leave it alone.
>>
>> What do you think?
>
> I'm not keen on having the attach listener thread periodically wakeup
> just to do this.

The required frequency would be very low. Once a day would be more than
enough. cron jobs are usually set-up to remove files that have not been
touched for several days.

> Can we not change permissions to protect the file form external
> deletion and ensure the VM cleans it up itself?

If VM crashes, the file is not deleted and it would be wrong if it
persisted forever (or until reboot) because of permissions. Besides, I
don't think there is a (portable, POSIX) permission that would prevent
removing the file since cron job executes as root to be able to remove
files created by several users. /tmp directory normally has "sticky" bit
set, meaning that anybody can create files in it (ugo+w permission), but
only owner of the file or owner of the dir or root can delete the file.

cron jobs that remove /tmp files are set-up for the following reason:
Processes usually clean-up after themselves but this is not guaranteed
if they terminate with a crash or unhandleable signal (such as KILL).

Regards, Peter

>
> David
>
>> Regards, Peter
>>
>>
>> On 6/20/19 10:49 PM, David Holmes wrote:
>>> Sorry it took me a while to understand the specifics of the problem. :)
>>>
>>> David
>>>
>>> On 20/06/2019 3:37 am, nijiaben wrote:
>>>> Yes Alan, I mean this
>>>> ------------------ Original ------------------
>>>> *From: * "Alan Bateman"<[hidden email]>;
>>>> *Date: * Thu, Jun 20, 2019 02:54 PM
>>>> *To: * "nijiaben"<[hidden email]>; "David
>>>> Holmes"<[hidden email]>;
>>>> "serviceability-dev"<[hidden email]>;
>>>> "jdk8u-dev"<[hidden email]>;
>>>> "hotspot-runtime-dev"<[hidden email]>;
>>>> *Subject: * Re: A Bug about the JVM Attach mechanism
>>>> On 20/06/2019 05:10, nijiaben wrote:
>>>>  > :
>>>>  > I know this mechanism, can we provide means of recovery to avoid
>>>> unavailability caused by accidental deletion?
>>>>  >
>>>> Are you concerned about tmpreaper or cron jobs that periodically
>>>> cleanup
>>>> /tmp? There may indeed be an issue for applications that run for weeks
>>>> or months. If someone is using jmap, jcmd or other tools using the
>>>> attach API then it will trigger the attach listener to start. When
>>>> they
>>>> come back in a few weeks then the .java_pid<pid> file may have been
>>>> removed so they cannot attach. Is this what what you are pointing out?
>>>>
>>>> -Alan
>>

Reply | Threaded
Open this post in threaded view
|

Re: A Bug about the JVM Attach mechanism

Chris Plummer
In reply to this post by David Holmes
On 6/21/19 12:14 PM, Peter Levart wrote:

> On 6/21/19 8:38 PM, Chris Plummer wrote:
>> On 6/21/19 8:57 AM, David Holmes wrote:
>>> Hi Peter,
>>>
>>> On 21/06/2019 7:55 am, Peter Levart wrote:
>>>> As far as I know, cron jobs that cleanup /tmp typically remove
>>>> files that have not been modified for a while.
>>>>
>>>> On Fedora for example, there is a systemd timer that triggers once
>>>> per day and executes systemd-tmpfiles which manages volatile and
>>>> temporary files and directories. The configuration for /tmp is the
>>>> following:
>>>>
>>>> # Clear tmp directories separately, to make them easier to override
>>>> q /tmp 1777 root root 10d
>>>> q /var/tmp 1777 root root 30d
>>>>
>>>> The age field (10 days for /tmp) has the following meaning:
>>>>
>>>>         The age of a file system entry is determined from its last
>>>> modification timestamp (mtime), its last access timestamp (atime),
>>>> and (except for directories) its last status change
>>>>         timestamp (ctime). Any of these three (or two) values will
>>>> prevent cleanup if it is more recent than the current time minus
>>>> the age field.
>>>>
>>>> So the solution could be for attach thread (if it is already
>>>> started) to update mtime or ctime of the .java_pid<pid> socket file
>>>> periodically so cleanup job would leave it alone.
>>>>
>>>> What do you think?
>>>
>>> I'm not keen on having the attach listener thread periodically
>>> wakeup just to do this. Can we not change permissions to protect the
>>> file form external deletion and ensure the VM cleans it up itself?
>> I think the JVM has a mechanism for cleaning up stale java_pid files
>> on startup (deleting those for which there is currently no java
>> process running). We need to make sure that changing permissions does
>> not break that.
>>
>> Rather than  having the JVM update mtime on the file, perhaps the
>> onus should be on the sys admin that is running scripts to clean up
>> these files in the first place. The scripts could first update mtime
>> on an java_pid files for which there is a java process still running.
>
> Problem is that those scripts are usually already set-up by default.
>
> OTOH it is relatively easy to create exceptions for files with a
> particular pattern. If /tmp/.java_pid* files are already managed by
> java launcher at every execution, then they need not be managed by a
> cron job. So there exists a workaround. It would only be more
> convenient if it was not needed.
I'm starting to second guess my thinking that there is any such
.java_pid removal support. I thought I saw it once, but can't find it
now. All I can find is that AttachListener::vm_start() will remove any
stale .java_pid file in place for the current pid. And judging by the
large collection of .java_pid files on my linux dev box, I doubt there
is any removal.

What I might have been remembering is perfdata file removal. In
/tmp/hsperfdata_<uid>/ I'm only seeing perfdata files for existing java
processes.

Chris

>
> Regards, Peter
>
>>
>> Chris
>>>
>>> David
>>>
>>>> Regards, Peter
>>>>
>>>>
>>>> On 6/20/19 10:49 PM, David Holmes wrote:
>>>>> Sorry it took me a while to understand the specifics of the
>>>>> problem. :)
>>>>>
>>>>> David
>>>>>
>>>>> On 20/06/2019 3:37 am, nijiaben wrote:
>>>>>> Yes Alan, I mean this
>>>>>> ------------------ Original ------------------
>>>>>> *From: * "Alan Bateman"<[hidden email]>;
>>>>>> *Date: * Thu, Jun 20, 2019 02:54 PM
>>>>>> *To: * "nijiaben"<[hidden email]>; "David
>>>>>> Holmes"<[hidden email]>;
>>>>>> "serviceability-dev"<[hidden email]>;
>>>>>> "jdk8u-dev"<[hidden email]>;
>>>>>> "hotspot-runtime-dev"<[hidden email]>;
>>>>>> *Subject: * Re: A Bug about the JVM Attach mechanism
>>>>>> On 20/06/2019 05:10, nijiaben wrote:
>>>>>>  > :
>>>>>>  > I know this mechanism, can we provide means of recovery to
>>>>>> avoid unavailability caused by accidental deletion?
>>>>>>  >
>>>>>> Are you concerned about tmpreaper or cron jobs that periodically
>>>>>> cleanup
>>>>>> /tmp? There may indeed be an issue for applications that run for
>>>>>> weeks
>>>>>> or months. If someone is using jmap, jcmd or other tools using the
>>>>>> attach API then it will trigger the attach listener to start.
>>>>>> When they
>>>>>> come back in a few weeks then the .java_pid<pid> file may have been
>>>>>> removed so they cannot attach. Is this what what you are pointing
>>>>>> out?
>>>>>>
>>>>>> -Alan
>>>>
>>
>>
>


Reply | Threaded
Open this post in threaded view
|

Re: A Bug about the JVM Attach mechanism

Peter Levart
In reply to this post by Peter Levart
Here's  a thread that describes why systemd-tmpfiles doesn't implement the --fuser option that tmpwatch does:

https://lists.freedesktop.org/archives/systemd-devel/2015-April/031339.html

The authors say the following:


In almost all cases the atime checks we do should be fully sufficient
though. Do you have a case where they aren't?

(Note that we do compare all unix sockets we find with /proc/net/unix
though, which is relatively efficient still. Also, as unix sockets
which are actively used do not get their atime bumped this is kinda a
necessity to cover them nicely.)


...meaning that /tmp/.java_pid<pid> which are UNIX sockets shouldn't be deleted by systemd-tmpfiles while still in use. Users of tmpwatch OTOH should enable the --fuser option.

Is this enough for you,  Nijiaben?


Regards, Peter

Reply | Threaded
Open this post in threaded view
|

Re: A Bug about the JVM Attach mechanism

Yasumasa Suenaga-4
In reply to this post by Yasumasa Suenaga-4
Hi,

 >> I'm not clear how this addresses the issue with deleting the file? The
 >> file still has to exist IIUC for the mechanism to work.
 >>
 >> This seems more suited to fixing the "multiple attach threads" problem.

How about this change?
   http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.2/

I think it can fix both JDK-8225690 and JDK-8225193.

This webrev implements for Linux only.
If we work further based on this, we need to implement
AttachListener::check_socket_file() for each OS's implementation.


Thanks,

Yasumasa


On 2019/06/21 23:16, Yasumasa Suenaga wrote:

> On 2019/06/21 22:51, David Holmes wrote:
>> Hi Yasumasa,
>>
>> On 21/06/2019 5:12 am, Yasumasa Suenaga wrote:
>>> Hi,
>>>
>>> Can we fix this issue like this webrev?
>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal/
>>
>> I'm not clear how this addresses the issue with deleting the file? The
>> file still has to exist IIUC for the mechanism to work.
>>
>> This seems more suited to fixing the "multiple attach threads" problem.
>
> Yes, my proposal focuses to "multiple attach threads".
> I shared my patch because it might help the work for this issue.
>
> This patch would guard multithread-issue in Attach Listener.
> So unix domain socket file will create just once.
>
>
> Yasumasa
>
>
>> David
>>
>>> I could reproduce this issue with ConcAttachTest.java in it
>>> on my laptop.
>>> (Fedora 30 x64 on Hyper-V guest: Core i3-8145U x 2vcpu)
>>>
>>> If we need to fix based on current implementation, I think
>>> we need to use atomic operation.
>>> But if we can refactor around here, we might be able to another
>>> approach - e.g. using monitors/mutexes
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2019/06/21 5:49, David Holmes wrote:
>>>> Sorry it took me a while to understand the specifics of the problem. :)
>>>>
>>>> David
>>>>
>>>> On 20/06/2019 3:37 am, nijiaben wrote:
>>>>> Yes Alan, I mean this
>>>>> ------------------?Original?------------------
>>>>> *From: *?"Alan Bateman"<Alan.Bateman at oracle.com>;
>>>>> *Date: *?Thu, Jun 20, 2019 02:54 PM
>>>>> *To: *?"nijiaben"<nijiaben at perfma.com>; "David
>>>>> Holmes"<david.holmes at oracle.com>;
>>>>> "serviceability-dev"<serviceability-dev at openjdk.java.net>;
>>>>> "jdk8u-dev"<jdk8u-dev at openjdk.java.net>;
>>>>> "hotspot-runtime-dev"<hotspot-runtime-dev at openjdk.java.net>;
>>>>> *Subject: *?Re: A Bug about the JVM Attach mechanism
>>>>> On 20/06/2019 05:10, nijiaben wrote:
>>>>>   > :
>>>>>   > I know this mechanism, can we provide means of recovery to avoid
>>>>> unavailability caused by accidental deletion?
>>>>>   >
>>>>> Are you concerned about tmpreaper or cron jobs that periodically
>>>>> cleanup
>>>>> /tmp? There may indeed be an issue for applications that run for weeks
>>>>> or months. If someone is using jmap, jcmd or other tools using the
>>>>> attach API then it will trigger the attach listener to start. When
>>>>> they
>>>>> come back in a few weeks then the .java_pid<pid> file may have been
>>>>> removed so they cannot attach. Is this what what you are pointing out?
>>>>>
>>>>> -Alan
Reply | Threaded
Open this post in threaded view
|

Re: A Bug about the JVM Attach mechanism

serguei.spitsyn@oracle.com
Hi Yasumasa,

I'm reviewing it.

Just a quick comment.

http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.2/src/hotspot/os/linux/attachListener_linux.cpp.udiff.html

+  // reads a request from the given connected socket
+  static LinuxAttachOperation* read_request(int s);
+
+  static bool _atexit_registered;
+
+ public:
+  enum {
+    ATTACH_PROTOCOL_VER = 1                     // protocol version
+  };
+  enum {
+    ATTACH_ERROR_BADVERSION     = 101           // error codes
+  };
+
   static void set_path(char* path) {
     if (path == NULL) {
+      _path[0] = '\0';
       _has_path = false;
     } else {
       strncpy(_path, path, UNIX_PATH_MAX);
       _path[UNIX_PATH_MAX-1] = '\0';
       _has_path = true;
     }
   }
 
   static void set_listener(int s)               { _listener = s; }
 
-  // reads a request from the given connected socket
-  static LinuxAttachOperation* read_request(int s);
-
- public:
-  enum {
-    ATTACH_PROTOCOL_VER = 1                     // protocol version
-  };
-  enum {
-    ATTACH_ERROR_BADVERSION     = 101           // error codes
-  };
-

You moved public definitions of enums up in the code.
All the declarations below that were private before became public now.
Not sure, if it was your intention

Also, the Copyright comment in this file needs an update.

Thanks,
Serguei



On 6/26/19 07:34, Yasumasa Suenaga wrote:
Hi,

>> I'm not clear how this addresses the issue with deleting the file? The
>> file still has to exist IIUC for the mechanism to work.
>>
>> This seems more suited to fixing the "multiple attach threads" problem.

How about this change?
  http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.2/

I think it can fix both JDK-8225690 and JDK-8225193.

This webrev implements for Linux only.
If we work further based on this, we need to implement AttachListener::check_socket_file() for each OS's implementation.


Thanks,

Yasumasa


On 2019/06/21 23:16, Yasumasa Suenaga wrote:
On 2019/06/21 22:51, David Holmes wrote:
Hi Yasumasa,

On 21/06/2019 5:12 am, Yasumasa Suenaga wrote:
Hi,

Can we fix this issue like this webrev?
   http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal/

I'm not clear how this addresses the issue with deleting the file? The file still has to exist IIUC for the mechanism to work.

This seems more suited to fixing the "multiple attach threads" problem.

Yes, my proposal focuses to "multiple attach threads".
I shared my patch because it might help the work for this issue.

This patch would guard multithread-issue in Attach Listener.
So unix domain socket file will create just once.


Yasumasa


David

I could reproduce this issue with ConcAttachTest.java in it
on my laptop.
(Fedora 30 x64 on Hyper-V guest: Core i3-8145U x 2vcpu)

If we need to fix based on current implementation, I think
we need to use atomic operation.
But if we can refactor around here, we might be able to another approach - e.g. using monitors/mutexes


Thanks,

Yasumasa


On 2019/06/21 5:49, David Holmes wrote:
Sorry it took me a while to understand the specifics of the problem. :)

David

On 20/06/2019 3:37 am, nijiaben wrote:
Yes Alan, I mean this
------------------?Original?------------------
*From: *?"Alan Bateman"<Alan.Bateman at oracle.com>;
*Date: *?Thu, Jun 20, 2019 02:54 PM
*To: *?"nijiaben"<nijiaben at perfma.com>; "David
Holmes"<david.holmes at oracle.com>;
"serviceability-dev"<serviceability-dev at openjdk.java.net>;
"jdk8u-dev"<jdk8u-dev at openjdk.java.net>;
"hotspot-runtime-dev"<hotspot-runtime-dev at openjdk.java.net>;
*Subject: *?Re: A Bug about the JVM Attach mechanism
On 20/06/2019 05:10, nijiaben wrote:
  > :
  > I know this mechanism, can we provide means of recovery to avoid
unavailability caused by accidental deletion?
  >
Are you concerned about tmpreaper or cron jobs that periodically cleanup
/tmp? There may indeed be an issue for applications that run for weeks
or months. If someone is using jmap, jcmd or other tools using the
attach API then it will trigger the attach listener to start. When they
come back in a few weeks then the .java_pid<pid> file may have been
removed so they cannot attach. Is this what what you are pointing out?

-Alan

Reply | Threaded
Open this post in threaded view
|

Re: A Bug about the JVM Attach mechanism

serguei.spitsyn@oracle.com
In reply to this post by Yasumasa Suenaga-4
Hi Yasumasa,

I'm reviewing it.

Just a quick comment.

http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.2/src/hotspot/os/linux/attachListener_linux.cpp.udiff.html

+  // reads a request from the given connected socket
+  static LinuxAttachOperation* read_request(int s);
+
+  static bool _atexit_registered;
+
+ public:
+  enum {
+    ATTACH_PROTOCOL_VER = 1                     // protocol version
+  };
+  enum {
+    ATTACH_ERROR_BADVERSION     = 101           // error codes
+  };
+
   static void set_path(char* path) {
     if (path == NULL) {
+      _path[0] = '\0';
       _has_path = false;
     } else {
       strncpy(_path, path, UNIX_PATH_MAX);
       _path[UNIX_PATH_MAX-1] = '\0';
       _has_path = true;
     }
   }
 
   static void set_listener(int s)               { _listener = s; }
 
-  // reads a request from the given connected socket
-  static LinuxAttachOperation* read_request(int s);
-
- public:
-  enum {
-    ATTACH_PROTOCOL_VER = 1                     // protocol version
-  };
-  enum {
-    ATTACH_ERROR_BADVERSION     = 101           // error codes
-  };
-

You moved public definitions of enums up in the code.
All the declarations below that were private before became public now.
Not sure, if it was your intention

Also, the Copyright comment in this file needs an update.

Thanks,
Serguei



On 6/26/19 07:34, Yasumasa Suenaga wrote:
Hi,

>> I'm not clear how this addresses the issue with deleting the file? The
>> file still has to exist IIUC for the mechanism to work.
>>
>> This seems more suited to fixing the "multiple attach threads" problem.

How about this change?
  http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.2/

I think it can fix both JDK-8225690 and JDK-8225193.

This webrev implements for Linux only.
If we work further based on this, we need to implement AttachListener::check_socket_file() for each OS's implementation.


Thanks,

Yasumasa


On 2019/06/21 23:16, Yasumasa Suenaga wrote:
On 2019/06/21 22:51, David Holmes wrote:
Hi Yasumasa,

On 21/06/2019 5:12 am, Yasumasa Suenaga wrote:
Hi,

Can we fix this issue like this webrev?
   http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal/

I'm not clear how this addresses the issue with deleting the file? The file still has to exist IIUC for the mechanism to work.

This seems more suited to fixing the "multiple attach threads" problem.

Yes, my proposal focuses to "multiple attach threads".
I shared my patch because it might help the work for this issue.

This patch would guard multithread-issue in Attach Listener.
So unix domain socket file will create just once.


Yasumasa


David

I could reproduce this issue with ConcAttachTest.java in it
on my laptop.
(Fedora 30 x64 on Hyper-V guest: Core i3-8145U x 2vcpu)

If we need to fix based on current implementation, I think
we need to use atomic operation.
But if we can refactor around here, we might be able to another approach - e.g. using monitors/mutexes


Thanks,

Yasumasa


On 2019/06/21 5:49, David Holmes wrote:
Sorry it took me a while to understand the specifics of the problem. :)

David

On 20/06/2019 3:37 am, nijiaben wrote:
Yes Alan, I mean this
------------------?Original?------------------
*From: *?"Alan Bateman"<Alan.Bateman at oracle.com>;
*Date: *?Thu, Jun 20, 2019 02:54 PM
*To: *?"nijiaben"<nijiaben at perfma.com>; "David
Holmes"<david.holmes at oracle.com>;
"serviceability-dev"<serviceability-dev at openjdk.java.net>;
"jdk8u-dev"<jdk8u-dev at openjdk.java.net>;
"hotspot-runtime-dev"<hotspot-runtime-dev at openjdk.java.net>;
*Subject: *?Re: A Bug about the JVM Attach mechanism
On 20/06/2019 05:10, nijiaben wrote:
  > :
  > I know this mechanism, can we provide means of recovery to avoid
unavailability caused by accidental deletion?
  >
Are you concerned about tmpreaper or cron jobs that periodically cleanup
/tmp? There may indeed be an issue for applications that run for weeks
or months. If someone is using jmap, jcmd or other tools using the
attach API then it will trigger the attach listener to start. When they
come back in a few weeks then the .java_pid<pid> file may have been
removed so they cannot attach. Is this what what you are pointing out?

-Alan

Reply | Threaded
Open this post in threaded view
|

Re: A Bug about the JVM Attach mechanism

Yasumasa Suenaga-4
Hi Serguei,

Thank you for your comment.
I uploaded new webrev which includes the fix.

  http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.3/

Also I added TODO for other Un*x OSes.
If the change for Linux is OK, I will copy it to others.
(I do not have them, so I depend on submit repo.)

This webrev has passed test/hotspot/jtreg/serviceability/attach and
test/jdk/com/sun/tools/attach jtreg tests on Linux x64.


Thanks,

Yasumasa

2019年6月27日(木) 6:10 [hidden email] <[hidden email]>:

>
> Hi Yasumasa,
>
> I'm reviewing it.
>
> Just a quick comment.
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.2/src/hotspot/os/linux/attachListener_linux.cpp.udiff.html
>
> +  // reads a request from the given connected socket
> +  static LinuxAttachOperation* read_request(int s);
> +
> +  static bool _atexit_registered;
> +
> + public:
> +  enum {
> +    ATTACH_PROTOCOL_VER = 1                     // protocol version
> +  };
> +  enum {
> +    ATTACH_ERROR_BADVERSION     = 101           // error codes
> +  };
> +
>    static void set_path(char* path) {
>      if (path == NULL) {
> +      _path[0] = '\0';
>        _has_path = false;
>      } else {
>        strncpy(_path, path, UNIX_PATH_MAX);
>        _path[UNIX_PATH_MAX-1] = '\0';
>        _has_path = true;
>      }
>    }
>
>    static void set_listener(int s)               { _listener = s; }
>
> -  // reads a request from the given connected socket
> -  static LinuxAttachOperation* read_request(int s);
> -
> - public:
> -  enum {
> -    ATTACH_PROTOCOL_VER = 1                     // protocol version
> -  };
> -  enum {
> -    ATTACH_ERROR_BADVERSION     = 101           // error codes
> -  };
> -
>
>
> You moved public definitions of enums up in the code.
> All the declarations below that were private before became public now.
> Not sure, if it was your intention
>
> Also, the Copyright comment in this file needs an update.
>
> Thanks,
> Serguei
>
>
>
> On 6/26/19 07:34, Yasumasa Suenaga wrote:
>
> Hi,
>
> >> I'm not clear how this addresses the issue with deleting the file? The
> >> file still has to exist IIUC for the mechanism to work.
> >>
> >> This seems more suited to fixing the "multiple attach threads" problem.
>
> How about this change?
>   http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.2/
>
> I think it can fix both JDK-8225690 and JDK-8225193.
>
> This webrev implements for Linux only.
> If we work further based on this, we need to implement AttachListener::check_socket_file() for each OS's implementation.
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2019/06/21 23:16, Yasumasa Suenaga wrote:
>
> On 2019/06/21 22:51, David Holmes wrote:
>
> Hi Yasumasa,
>
> On 21/06/2019 5:12 am, Yasumasa Suenaga wrote:
>
> Hi,
>
> Can we fix this issue like this webrev?
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal/
>
>
> I'm not clear how this addresses the issue with deleting the file? The file still has to exist IIUC for the mechanism to work.
>
> This seems more suited to fixing the "multiple attach threads" problem.
>
>
> Yes, my proposal focuses to "multiple attach threads".
> I shared my patch because it might help the work for this issue.
>
> This patch would guard multithread-issue in Attach Listener.
> So unix domain socket file will create just once.
>
>
> Yasumasa
>
>
> David
>
> I could reproduce this issue with ConcAttachTest.java in it
> on my laptop.
> (Fedora 30 x64 on Hyper-V guest: Core i3-8145U x 2vcpu)
>
> If we need to fix based on current implementation, I think
> we need to use atomic operation.
> But if we can refactor around here, we might be able to another approach - e.g. using monitors/mutexes
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2019/06/21 5:49, David Holmes wrote:
>
> Sorry it took me a while to understand the specifics of the problem. :)
>
> David
>
> On 20/06/2019 3:37 am, nijiaben wrote:
>
> Yes Alan, I mean this
> ------------------?Original?------------------
> *From: *?"Alan Bateman"<Alan.Bateman at oracle.com>;
> *Date: *?Thu, Jun 20, 2019 02:54 PM
> *To: *?"nijiaben"<nijiaben at perfma.com>; "David
> Holmes"<david.holmes at oracle.com>;
> "serviceability-dev"<serviceability-dev at openjdk.java.net>;
> "jdk8u-dev"<jdk8u-dev at openjdk.java.net>;
> "hotspot-runtime-dev"<hotspot-runtime-dev at openjdk.java.net>;
> *Subject: *?Re: A Bug about the JVM Attach mechanism
> On 20/06/2019 05:10, nijiaben wrote:
>   > :
>   > I know this mechanism, can we provide means of recovery to avoid
> unavailability caused by accidental deletion?
>   >
> Are you concerned about tmpreaper or cron jobs that periodically cleanup
> /tmp? There may indeed be an issue for applications that run for weeks
> or months. If someone is using jmap, jcmd or other tools using the
> attach API then it will trigger the attach listener to start. When they
> come back in a few weeks then the .java_pid<pid> file may have been
> removed so they cannot attach. Is this what what you are pointing out?
>
> -Alan
>
>
Reply | Threaded
Open this post in threaded view
|

Re: A Bug about the JVM Attach mechanism

serguei.spitsyn@oracle.com
Hi Yasumasa,

The approach looks Okay in general.
Could you, please, post an RFR with the bug number in the email subject?

Thanks,
Serguei


On 6/26/19 18:41, Yasumasa Suenaga wrote:

> Hi Serguei,
>
> Thank you for your comment.
> I uploaded new webrev which includes the fix.
>
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.3/
>
> Also I added TODO for other Un*x OSes.
> If the change for Linux is OK, I will copy it to others.
> (I do not have them, so I depend on submit repo.)
>
> This webrev has passed test/hotspot/jtreg/serviceability/attach and
> test/jdk/com/sun/tools/attach jtreg tests on Linux x64.
>
>
> Thanks,
>
> Yasumasa
>
> 2019年6月27日(木) 6:10 [hidden email] <[hidden email]>:
>> Hi Yasumasa,
>>
>> I'm reviewing it.
>>
>> Just a quick comment.
>>
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.2/src/hotspot/os/linux/attachListener_linux.cpp.udiff.html
>>
>> +  // reads a request from the given connected socket
>> +  static LinuxAttachOperation* read_request(int s);
>> +
>> +  static bool _atexit_registered;
>> +
>> + public:
>> +  enum {
>> +    ATTACH_PROTOCOL_VER = 1                     // protocol version
>> +  };
>> +  enum {
>> +    ATTACH_ERROR_BADVERSION     = 101           // error codes
>> +  };
>> +
>>     static void set_path(char* path) {
>>       if (path == NULL) {
>> +      _path[0] = '\0';
>>         _has_path = false;
>>       } else {
>>         strncpy(_path, path, UNIX_PATH_MAX);
>>         _path[UNIX_PATH_MAX-1] = '\0';
>>         _has_path = true;
>>       }
>>     }
>>
>>     static void set_listener(int s)               { _listener = s; }
>>
>> -  // reads a request from the given connected socket
>> -  static LinuxAttachOperation* read_request(int s);
>> -
>> - public:
>> -  enum {
>> -    ATTACH_PROTOCOL_VER = 1                     // protocol version
>> -  };
>> -  enum {
>> -    ATTACH_ERROR_BADVERSION     = 101           // error codes
>> -  };
>> -
>>
>>
>> You moved public definitions of enums up in the code.
>> All the declarations below that were private before became public now.
>> Not sure, if it was your intention
>>
>> Also, the Copyright comment in this file needs an update.
>>
>> Thanks,
>> Serguei
>>
>>
>>
>> On 6/26/19 07:34, Yasumasa Suenaga wrote:
>>
>> Hi,
>>
>>>> I'm not clear how this addresses the issue with deleting the file? The
>>>> file still has to exist IIUC for the mechanism to work.
>>>>
>>>> This seems more suited to fixing the "multiple attach threads" problem.
>> How about this change?
>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.2/
>>
>> I think it can fix both JDK-8225690 and JDK-8225193.
>>
>> This webrev implements for Linux only.
>> If we work further based on this, we need to implement AttachListener::check_socket_file() for each OS's implementation.
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> On 2019/06/21 23:16, Yasumasa Suenaga wrote:
>>
>> On 2019/06/21 22:51, David Holmes wrote:
>>
>> Hi Yasumasa,
>>
>> On 21/06/2019 5:12 am, Yasumasa Suenaga wrote:
>>
>> Hi,
>>
>> Can we fix this issue like this webrev?
>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal/
>>
>>
>> I'm not clear how this addresses the issue with deleting the file? The file still has to exist IIUC for the mechanism to work.
>>
>> This seems more suited to fixing the "multiple attach threads" problem.
>>
>>
>> Yes, my proposal focuses to "multiple attach threads".
>> I shared my patch because it might help the work for this issue.
>>
>> This patch would guard multithread-issue in Attach Listener.
>> So unix domain socket file will create just once.
>>
>>
>> Yasumasa
>>
>>
>> David
>>
>> I could reproduce this issue with ConcAttachTest.java in it
>> on my laptop.
>> (Fedora 30 x64 on Hyper-V guest: Core i3-8145U x 2vcpu)
>>
>> If we need to fix based on current implementation, I think
>> we need to use atomic operation.
>> But if we can refactor around here, we might be able to another approach - e.g. using monitors/mutexes
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> On 2019/06/21 5:49, David Holmes wrote:
>>
>> Sorry it took me a while to understand the specifics of the problem. :)
>>
>> David
>>
>> On 20/06/2019 3:37 am, nijiaben wrote:
>>
>> Yes Alan, I mean this
>> ------------------?Original?------------------
>> *From: *?"Alan Bateman"<Alan.Bateman at oracle.com>;
>> *Date: *?Thu, Jun 20, 2019 02:54 PM
>> *To: *?"nijiaben"<nijiaben at perfma.com>; "David
>> Holmes"<david.holmes at oracle.com>;
>> "serviceability-dev"<serviceability-dev at openjdk.java.net>;
>> "jdk8u-dev"<jdk8u-dev at openjdk.java.net>;
>> "hotspot-runtime-dev"<hotspot-runtime-dev at openjdk.java.net>;
>> *Subject: *?Re: A Bug about the JVM Attach mechanism
>> On 20/06/2019 05:10, nijiaben wrote:
>>    > :
>>    > I know this mechanism, can we provide means of recovery to avoid
>> unavailability caused by accidental deletion?
>>    >
>> Are you concerned about tmpreaper or cron jobs that periodically cleanup
>> /tmp? There may indeed be an issue for applications that run for weeks
>> or months. If someone is using jmap, jcmd or other tools using the
>> attach API then it will trigger the attach listener to start. When they
>> come back in a few weeks then the .java_pid<pid> file may have been
>> removed so they cannot attach. Is this what what you are pointing out?
>>
>> -Alan
>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: A Bug about the JVM Attach mechanism

Yasumasa Suenaga-4
Hi Serguei,

Can I send RFR for JDK-8225690 and close JDK-8225193 as duplicate of it?


Thanks,

Yasumasa


On 2019/07/03 1:06, [hidden email] wrote:

> Hi Yasumasa,
>
> The approach looks Okay in general.
> Could you, please, post an RFR with the bug number in the email subject?
>
> Thanks,
> Serguei
>
>
> On 6/26/19 18:41, Yasumasa Suenaga wrote:
>> Hi Serguei,
>>
>> Thank you for your comment.
>> I uploaded new webrev which includes the fix.
>>
>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.3/
>>
>> Also I added TODO for other Un*x OSes.
>> If the change for Linux is OK, I will copy it to others.
>> (I do not have them, so I depend on submit repo.)
>>
>> This webrev has passed test/hotspot/jtreg/serviceability/attach and
>> test/jdk/com/sun/tools/attach jtreg tests on Linux x64.
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>> 2019年6月27日(木) 6:10 [hidden email] <[hidden email]>:
>>> Hi Yasumasa,
>>>
>>> I'm reviewing it.
>>>
>>> Just a quick comment.
>>>
>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.2/src/hotspot/os/linux/attachListener_linux.cpp.udiff.html
>>>
>>> +  // reads a request from the given connected socket
>>> +  static LinuxAttachOperation* read_request(int s);
>>> +
>>> +  static bool _atexit_registered;
>>> +
>>> + public:
>>> +  enum {
>>> +    ATTACH_PROTOCOL_VER = 1                     // protocol version
>>> +  };
>>> +  enum {
>>> +    ATTACH_ERROR_BADVERSION     = 101           // error codes
>>> +  };
>>> +
>>>     static void set_path(char* path) {
>>>       if (path == NULL) {
>>> +      _path[0] = '\0';
>>>         _has_path = false;
>>>       } else {
>>>         strncpy(_path, path, UNIX_PATH_MAX);
>>>         _path[UNIX_PATH_MAX-1] = '\0';
>>>         _has_path = true;
>>>       }
>>>     }
>>>
>>>     static void set_listener(int s)               { _listener = s; }
>>>
>>> -  // reads a request from the given connected socket
>>> -  static LinuxAttachOperation* read_request(int s);
>>> -
>>> - public:
>>> -  enum {
>>> -    ATTACH_PROTOCOL_VER = 1                     // protocol version
>>> -  };
>>> -  enum {
>>> -    ATTACH_ERROR_BADVERSION     = 101           // error codes
>>> -  };
>>> -
>>>
>>>
>>> You moved public definitions of enums up in the code.
>>> All the declarations below that were private before became public now.
>>> Not sure, if it was your intention
>>>
>>> Also, the Copyright comment in this file needs an update.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>
>>> On 6/26/19 07:34, Yasumasa Suenaga wrote:
>>>
>>> Hi,
>>>
>>>>> I'm not clear how this addresses the issue with deleting the file? The
>>>>> file still has to exist IIUC for the mechanism to work.
>>>>>
>>>>> This seems more suited to fixing the "multiple attach threads" problem.
>>> How about this change?
>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal.2/
>>>
>>> I think it can fix both JDK-8225690 and JDK-8225193.
>>>
>>> This webrev implements for Linux only.
>>> If we work further based on this, we need to implement AttachListener::check_socket_file() for each OS's implementation.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2019/06/21 23:16, Yasumasa Suenaga wrote:
>>>
>>> On 2019/06/21 22:51, David Holmes wrote:
>>>
>>> Hi Yasumasa,
>>>
>>> On 21/06/2019 5:12 am, Yasumasa Suenaga wrote:
>>>
>>> Hi,
>>>
>>> Can we fix this issue like this webrev?
>>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/proposal/
>>>
>>>
>>> I'm not clear how this addresses the issue with deleting the file? The file still has to exist IIUC for the mechanism to work.
>>>
>>> This seems more suited to fixing the "multiple attach threads" problem.
>>>
>>>
>>> Yes, my proposal focuses to "multiple attach threads".
>>> I shared my patch because it might help the work for this issue.
>>>
>>> This patch would guard multithread-issue in Attach Listener.
>>> So unix domain socket file will create just once.
>>>
>>>
>>> Yasumasa
>>>
>>>
>>> David
>>>
>>> I could reproduce this issue with ConcAttachTest.java in it
>>> on my laptop.
>>> (Fedora 30 x64 on Hyper-V guest: Core i3-8145U x 2vcpu)
>>>
>>> If we need to fix based on current implementation, I think
>>> we need to use atomic operation.
>>> But if we can refactor around here, we might be able to another approach - e.g. using monitors/mutexes
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2019/06/21 5:49, David Holmes wrote:
>>>
>>> Sorry it took me a while to understand the specifics of the problem. :)
>>>
>>> David
>>>
>>> On 20/06/2019 3:37 am, nijiaben wrote:
>>>
>>> Yes Alan, I mean this
>>> ------------------?Original?------------------
>>> *From: *?"Alan Bateman"<Alan.Bateman at oracle.com>;
>>> *Date: *?Thu, Jun 20, 2019 02:54 PM
>>> *To: *?"nijiaben"<nijiaben at perfma.com>; "David
>>> Holmes"<david.holmes at oracle.com>;
>>> "serviceability-dev"<serviceability-dev at openjdk.java.net>;
>>> "jdk8u-dev"<jdk8u-dev at openjdk.java.net>;
>>> "hotspot-runtime-dev"<hotspot-runtime-dev at openjdk.java.net>;
>>> *Subject: *?Re: A Bug about the JVM Attach mechanism
>>> On 20/06/2019 05:10, nijiaben wrote:
>>>    > :
>>>    > I know this mechanism, can we provide means of recovery to avoid
>>> unavailability caused by accidental deletion?
>>>    >
>>> Are you concerned about tmpreaper or cron jobs that periodically cleanup
>>> /tmp? There may indeed be an issue for applications that run for weeks
>>> or months. If someone is using jmap, jcmd or other tools using the
>>> attach API then it will trigger the attach listener to start. When they
>>> come back in a few weeks then the .java_pid<pid> file may have been
>>> removed so they cannot attach. Is this what what you are pointing out?
>>>
>>> -Alan
>>>
>>>
>