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.hol...@oracle.com>; Shafi Ahmad
<shafi.s.ah...@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.ah...@oracle.com>
<mailto:shafi.s.ah...@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
