[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: [10] RFR for JDK-8170299: Debugger does not stop inside the low memory notifications code [inter
From:       David Holmes <david.holmes () oracle ! com>
Date:       2017-12-15 2:20:51
Message-ID: e48d0e2c-bfaf-dc96-9b66-0a7a857d0ff2 () oracle ! com
[Download RAW message or body]

On 13/12/2017 8:23 PM, Shafi Ahmad wrote:
> Thank you Mandy and David for review comments.
> 
> Please find updated webrev: 
> http://cr.openjdk.java.net/~shshahma/8170299/webrev.03/
> 
> I have modify the code to use Emitter class rather than Broadcaster.

Okay. This at least seems less intrusive/disruptive.

Not sure why you added this:

  180     protected void handleNotification(NotificationListener listener,
  181                                       Notification notif, Object 
handback) {
  182         listener.handleNotification(notif, handback);
  183     }

instead of executing line 182 directly at 228?

  226         public void run() {
  227             try {
  228                 handleNotification(listenerInfo.listener,
  229                                    notif, listenerInfo.handback);
  230             } catch (Exception e) {

I'm still concerned that the introduction of a new thread to actually 
execute the notification will causes more problems than it solves:
- the notifications are no longer synchronous wrt. sendNotification
- the execution context (security credentials etc) may be different in 
the InnocuousThread compared to the service thread.
- there may be synchronization/deadlock issues

Despite the spec for NotificationEmitter stating:

"Implementations of this interface can differ regarding the thread in 
which the methods of filters and listeners are called."

this may be a significant behavioural change - just to fix a debugger issue.

Thanks,
David

> Regards,
> 
> Shafi
> 
> *From:*mandy chung
> *Sent:* Tuesday, December 12, 2017 12:06 PM
> *To:* David Holmes <david.holmes@oracle.com>; Shafi Ahmad 
> <shafi.s.ahmad@oracle.com>
> *Cc:* serviceability-dev@openjdk.java.net
> *Subject:* Re: [10] RFR for JDK-8170299: Debugger does not stop inside 
> the low memory notifications code [internal]
> 
> On 12/11/17 9:32 PM, David Holmes wrote:
> 
>     Hi Shafi,
> 
>     I missed the first round of reviews on this so hadn't seen previous
>     discussion. I'm still uneasy about introducing a new thread to the
>     mix here, but at least Mandy's suggestion to modify the *Emitter
>     class rather than change to the *Broadcaster is a lot less disruptive:
> 
>     http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021851.html
> 
> 
> 
> Yes this is a better alternative (thanks for bringing this up, David).
> 
> Mandy
> 
> 
>     David
> 
>     On 12/12/2017 3:02 PM, Shafi Ahmad wrote:
> 
>         Thank you Mandy and David.
> 
>         I am sorry, somehow I missed your earlier comment regarding the
>         RFE.
> 
>           >> This is a spec change.  Is this intentional?
> 
>         Yes, this change is required as this is referenced inside
>           MemoryImpl.java.
> 
>         I will file a separate RFE for adding hasListeners().
> 
>         I will send the updated webrev after incorporating the review
>         comment.
> 
>         Regards,
> 
>         Shafi
> 
>         *From:*mandy chung
>         *Sent:* Tuesday, December 12, 2017 2:35 AM
>         *To:* Shafi Ahmad <shafi.s.ahmad@oracle.com>
>         <mailto:shafi.s.ahmad@oracle.com>
>         *Cc:* serviceability-dev@openjdk.java.net
>         <mailto:serviceability-dev@openjdk.java.net>
>         *Subject:* Re: [10] RFR for JDK-8170299: Debugger does not stop
>         inside the low memory notifications code [internal]
> 
>         On 12/11/17 2:31 AM, Shafi Ahmad wrote:
> 
>              The method hasListeners() is referenced  inside
>         MemoryImpl.java and
>              defined in  NotificationEmitterSupport.
> 
>              This method is not present in
>           NotificationBroadcasterSupport so I
>              added it to NotificationBroadcasterSupport.
> 
> 
>         This is a spec change.  Is this intentional?  I replied in Sept
>         in reviewing an earlier version [1] that this cannot be
>         backported.  If you intend to make this spec change, it's better
>         to separate this RFE and it requires CSR.
> 
> 
>              Jdk10 bug: https://bugs.openjdk.java.net/browse/JDK-8170299
> 
>              Webrev link:
>         http://cr.openjdk.java.net/~shshahma/8170299/webrev.02/
>         <http://cr.openjdk.java.net/%7Eshshahma/8170299/webrev.02/>
>         <http://cr.openjdk.java.net/%7Eshshahma/8170299/webrev.02/>
> 
> 
>         MemoryImpl.java
> 
>         189                     th.setName("Debugger");
> 
>         this is not a debugger thread.  Maybe rename it to
> 
>         "MemoryPool notification thread"?
> 
>         Mandy
> 
>         [1]
>         http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021836.html
> 
[prev in list] [next in list] [prev in thread] [next in thread]