You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@river.apache.org by Patricia Shanahan <pa...@acm.org> on 2011/04/03 05:35:03 UTC
Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can
terminate discovery notifier threads
In reviewing the code prior to applying the patch, I noticed the
following in the LookupDiscoveryListener.Notifier.run() method:
try {
loc = regs[i].getLocator();
} catch (Throwable ex) { /* ignore */ }
What do people think about ignoring Throwable?
Chris, Thanks for the patch. Do you happen to have a unit or QA test for
this problem?
Patricia
On 3/30/2011 3:05 PM, Chris Dolan (JIRA) wrote:
> Ill-behaved DiscoveryListener can terminate discovery notifier threads
> ----------------------------------------------------------------------
>
> Key: RIVER-395
> URL: https://issues.apache.org/jira/browse/RIVER-395
> Project: River
> Issue Type: Bug
> Components: net_jini_discovery
> Affects Versions: jtsk_2.1
> Reporter: Chris Dolan
>
>
> (bug detected in Jini 2.1, still present in 2.1.2+ trunk)
>
> If a net.jini.discovery.DiscoveryListener implementation throws an unchecked exception, then the LookupLocatorDiscovery$Notifier thread and/or the LookupDiscovery$Notifier thread will exit prematurely. In practice, this can prevent the JoinManager$DiscMgrListener or ServiceDiscoveryManager$DiscMgrListener callbacks from being invoked, resulting in incomplete state for a registrar.
>
> A soon-to-be attached patch surrounds each listener invocation with a try/catch block.
>
> --
> This message is automatically generated by JIRA.
> For more information on JIRA, see: http://www.atlassian.com/software/jira
>
Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can
terminate discovery notifier threads
Posted by Tom Hobbs <tv...@googlemail.com>.
I agree. If your going to have a noisy log then you may as well have a
useful noisy log!
On 5 Apr 2011 08:40, "Dan Creswell" <da...@gmail.com> wrote:
> I think we should make the change to logging to avoid this confusion. It's
> straightforward to do, low impact and we needn't do it across the entire
> codebase all at once. Just make things better over time....
>
> On 4 April 2011 23:55, Patricia Shanahan <pa...@acm.org> wrote:
>
>> I'm having a lot of trouble making up my mind about this. I understand
your
>> comment, but here is the other side of it.
>>
>> FINEST logging means that someone really wants to find out what is going
on
>> in this area, probably because it may be involved in some bug.
>>
>> As the code is now, the only indication of the Throwable will be a "null"
>> in the log message.
>>
>> If we are lucky, the programmer will notice the null, read the code, see
>> that it could be due to a throw as well as a null getLocator() result,
>> modify the logging code to report the Throwable, and the problem will
>> reproduce the same way on the next run. In that case, we will have only
>> wasted one run.
>>
>> If we are unlucky, the programmer will interpret "locator = null" in the
>> log as meaning that the locator was null at that point, and be
unnecessarily
>> confused.
>>
>> Patricia
>>
>>
>>
>> On 4/4/2011 8:14 AM, Christopher Dolan wrote:
>>
>>> In this specific case, the Throwable is not relevant and not worth
>>> logging because the locator is only used for a Level.FINEST log message!
>>> More context from LookupLocatorDiscover.Notifier.run(), where the
>>> catch(Throwable) is in the middle:
>>>
>>> /* Log the event info about the lookup(s) */
>>> if(firstListener&& (logger.isLoggable(Level.FINEST)) ) {
>>> String eType = (task.discard ?
>>> "discarded":"discovered");
>>> ServiceRegistrar[] regs = e.getRegistrars();
>>> logger.finest(eType+" event -- "+regs.length
>>> +" lookup(s)");
>>> Map groupsMap = e.getGroups();
>>> for(int i=0;i<regs.length;i++) {
>>> LookupLocator loc = null;
>>> try {
>>> loc = regs[i].getLocator();
>>> } catch (Throwable ex) { /* ignore */ }
>>> String[] groups = (String[])groupsMap.get(regs[i]);
>>> logger.finest(" "+eType+" locator = "+loc);
>>> if(groups.length == 0) {
>>> logger.finest(" "+eType
>>> +" group = NO_GROUPS");
>>> } else {
>>> for(int j=0;j<groups.length;j++) {
>>> logger.finest(" "+eType+" group["+j+"] "
>>> +"= "+groups[j]);
>>> }//end loop
>>> }//endif(groups.length)
>>> }//end loop
>>> }//endif(firstListener&& isLoggable(Level.FINEST)
>>>
>>>
>>> There's nearly identical code in LookupDiscover.Notifier.run()
>>>
>>> Chris
>>>
>>> -----Original Message-----
>>> From: Patricia Shanahan [mailto:pats@acm.org]
>>> Sent: Monday, April 04, 2011 7:20 AM
>>> To: dev@river.apache.org
>>> Subject: Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener
>>> can terminate discovery notifier threads
>>>
>>> I understand Tom's feeling against just logging, but I think it is
>>> probably the best option for now. Once we log, we should be able to find
>>>
>>> out if this is an issue, and if there are cases that are happening that
>>> would benefit from some other action.
>>>
>>> My really strong objection is to *silently* catching and carrying on.
>>> Partly, that is a result of having done a lot of debug, some of which
>>> was made unnecessarily difficult by software that destroyed clues.
>>>
>>> Patricia
>>>
>>>
>>> On 4/4/2011 2:15 AM, Tom Hobbs wrote:
>>>
>>>> You're right about InvocationHandler I should probably wake up before
>>>>
>>> I send
>>>
>>>> emails.
>>>>
>>>> If the spec says that all "good" code throws ServerError we can leave
>>>>
>>> that
>>>
>>>> Throwable catch in as well. This way we know that any of the latter
>>>>
>>> means a
>>>
>>>> dos attack, non spec compliant services or something equally awful.
>>>>
>>>> I'm really reluctant to just leave a log and Throwable catch in; it
>>>>
>>> just
>>>
>>>> feels wrong. I guess we might have to though since writing code for
>>>>
>>> this
>>>
>>>> level requires a slightly different way of think than when at the
>>>> application level. I'm not going to keep flogging this dead horse
>>>>
>>> though, I
>>>
>>>> trust your judgement on this more than mine. :-)
>>>>
>>>> Tom
>>>>
>>>> On 4 Apr 2011 09:44, "Dan Creswell"<da...@gmail.com> wrote:
>>>> Can't do anything about the Throwable as it's part of
>>>>
>>> InvocationHandler and
>>>
>>>> that's the JDK spec.
>>>>
>>>> Could agree that our Dispatcher's only ever throw some specific
>>>>
>>> subclasses.
>>>
>>>> We'd have to do some diligence on that as BasicInvocationDispatcher
>>>>
>>> and
>>>
>>>> friends are designed to follow RMI spec, not entirely sure all other
>>>> transports can do enough in that respect to be compliant.
>>>>
>>>> There is one other problem with this however which is that badly
>>>>
>>> written
>>>
>>>> service code could chuck out stuff that is not compliant and bring
>>>>
>>> down the
>>>
>>>> entire house - that's kind of denial of service territory....
>>>>
>>>> ...personally I'd rather leave the catch throwable, log at some
>>>>
>>> suitable
>>>
>>>> level and leave it at that, at least until we gather some data as to
>>>>
>>> how
>>>
>>>> often this problem bites us etc.
>>>>
>>>>
>>>> On 4 April 2011 09:07, Tom Hobbs<tv...@googlemail.com> wrote:
>>>>
>>>> Thanks for the info, Dan. Of c...
>>>>>
>>>>
>>>>
>>>
>>>
>>
Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can
terminate discovery notifier threads
Posted by Dan Creswell <da...@gmail.com>.
I think we should make the change to logging to avoid this confusion. It's
straightforward to do, low impact and we needn't do it across the entire
codebase all at once. Just make things better over time....
On 4 April 2011 23:55, Patricia Shanahan <pa...@acm.org> wrote:
> I'm having a lot of trouble making up my mind about this. I understand your
> comment, but here is the other side of it.
>
> FINEST logging means that someone really wants to find out what is going on
> in this area, probably because it may be involved in some bug.
>
> As the code is now, the only indication of the Throwable will be a "null"
> in the log message.
>
> If we are lucky, the programmer will notice the null, read the code, see
> that it could be due to a throw as well as a null getLocator() result,
> modify the logging code to report the Throwable, and the problem will
> reproduce the same way on the next run. In that case, we will have only
> wasted one run.
>
> If we are unlucky, the programmer will interpret "locator = null" in the
> log as meaning that the locator was null at that point, and be unnecessarily
> confused.
>
> Patricia
>
>
>
> On 4/4/2011 8:14 AM, Christopher Dolan wrote:
>
>> In this specific case, the Throwable is not relevant and not worth
>> logging because the locator is only used for a Level.FINEST log message!
>> More context from LookupLocatorDiscover.Notifier.run(), where the
>> catch(Throwable) is in the middle:
>>
>> /* Log the event info about the lookup(s) */
>> if(firstListener&& (logger.isLoggable(Level.FINEST)) ) {
>> String eType = (task.discard ?
>> "discarded":"discovered");
>> ServiceRegistrar[] regs = e.getRegistrars();
>> logger.finest(eType+" event -- "+regs.length
>> +" lookup(s)");
>> Map groupsMap = e.getGroups();
>> for(int i=0;i<regs.length;i++) {
>> LookupLocator loc = null;
>> try {
>> loc = regs[i].getLocator();
>> } catch (Throwable ex) { /* ignore */ }
>> String[] groups = (String[])groupsMap.get(regs[i]);
>> logger.finest(" "+eType+" locator = "+loc);
>> if(groups.length == 0) {
>> logger.finest(" "+eType
>> +" group = NO_GROUPS");
>> } else {
>> for(int j=0;j<groups.length;j++) {
>> logger.finest(" "+eType+" group["+j+"] "
>> +"= "+groups[j]);
>> }//end loop
>> }//endif(groups.length)
>> }//end loop
>> }//endif(firstListener&& isLoggable(Level.FINEST)
>>
>>
>> There's nearly identical code in LookupDiscover.Notifier.run()
>>
>> Chris
>>
>> -----Original Message-----
>> From: Patricia Shanahan [mailto:pats@acm.org]
>> Sent: Monday, April 04, 2011 7:20 AM
>> To: dev@river.apache.org
>> Subject: Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener
>> can terminate discovery notifier threads
>>
>> I understand Tom's feeling against just logging, but I think it is
>> probably the best option for now. Once we log, we should be able to find
>>
>> out if this is an issue, and if there are cases that are happening that
>> would benefit from some other action.
>>
>> My really strong objection is to *silently* catching and carrying on.
>> Partly, that is a result of having done a lot of debug, some of which
>> was made unnecessarily difficult by software that destroyed clues.
>>
>> Patricia
>>
>>
>> On 4/4/2011 2:15 AM, Tom Hobbs wrote:
>>
>>> You're right about InvocationHandler I should probably wake up before
>>>
>> I send
>>
>>> emails.
>>>
>>> If the spec says that all "good" code throws ServerError we can leave
>>>
>> that
>>
>>> Throwable catch in as well. This way we know that any of the latter
>>>
>> means a
>>
>>> dos attack, non spec compliant services or something equally awful.
>>>
>>> I'm really reluctant to just leave a log and Throwable catch in; it
>>>
>> just
>>
>>> feels wrong. I guess we might have to though since writing code for
>>>
>> this
>>
>>> level requires a slightly different way of think than when at the
>>> application level. I'm not going to keep flogging this dead horse
>>>
>> though, I
>>
>>> trust your judgement on this more than mine. :-)
>>>
>>> Tom
>>>
>>> On 4 Apr 2011 09:44, "Dan Creswell"<da...@gmail.com> wrote:
>>> Can't do anything about the Throwable as it's part of
>>>
>> InvocationHandler and
>>
>>> that's the JDK spec.
>>>
>>> Could agree that our Dispatcher's only ever throw some specific
>>>
>> subclasses.
>>
>>> We'd have to do some diligence on that as BasicInvocationDispatcher
>>>
>> and
>>
>>> friends are designed to follow RMI spec, not entirely sure all other
>>> transports can do enough in that respect to be compliant.
>>>
>>> There is one other problem with this however which is that badly
>>>
>> written
>>
>>> service code could chuck out stuff that is not compliant and bring
>>>
>> down the
>>
>>> entire house - that's kind of denial of service territory....
>>>
>>> ...personally I'd rather leave the catch throwable, log at some
>>>
>> suitable
>>
>>> level and leave it at that, at least until we gather some data as to
>>>
>> how
>>
>>> often this problem bites us etc.
>>>
>>>
>>> On 4 April 2011 09:07, Tom Hobbs<tv...@googlemail.com> wrote:
>>>
>>> Thanks for the info, Dan. Of c...
>>>>
>>>
>>>
>>
>>
>
Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can
terminate discovery notifier threads
Posted by Patricia Shanahan <pa...@acm.org>.
I'm having a lot of trouble making up my mind about this. I understand
your comment, but here is the other side of it.
FINEST logging means that someone really wants to find out what is going
on in this area, probably because it may be involved in some bug.
As the code is now, the only indication of the Throwable will be a
"null" in the log message.
If we are lucky, the programmer will notice the null, read the code, see
that it could be due to a throw as well as a null getLocator() result,
modify the logging code to report the Throwable, and the problem will
reproduce the same way on the next run. In that case, we will have only
wasted one run.
If we are unlucky, the programmer will interpret "locator = null" in the
log as meaning that the locator was null at that point, and be
unnecessarily confused.
Patricia
On 4/4/2011 8:14 AM, Christopher Dolan wrote:
> In this specific case, the Throwable is not relevant and not worth
> logging because the locator is only used for a Level.FINEST log message!
> More context from LookupLocatorDiscover.Notifier.run(), where the
> catch(Throwable) is in the middle:
>
> /* Log the event info about the lookup(s) */
> if(firstListener&& (logger.isLoggable(Level.FINEST)) ) {
> String eType = (task.discard ?
> "discarded":"discovered");
> ServiceRegistrar[] regs = e.getRegistrars();
> logger.finest(eType+" event -- "+regs.length
> +" lookup(s)");
> Map groupsMap = e.getGroups();
> for(int i=0;i<regs.length;i++) {
> LookupLocator loc = null;
> try {
> loc = regs[i].getLocator();
> } catch (Throwable ex) { /* ignore */ }
> String[] groups = (String[])groupsMap.get(regs[i]);
> logger.finest(" "+eType+" locator = "+loc);
> if(groups.length == 0) {
> logger.finest(" "+eType
> +" group = NO_GROUPS");
> } else {
> for(int j=0;j<groups.length;j++) {
> logger.finest(" "+eType+" group["+j+"] "
> +"= "+groups[j]);
> }//end loop
> }//endif(groups.length)
> }//end loop
> }//endif(firstListener&& isLoggable(Level.FINEST)
>
>
> There's nearly identical code in LookupDiscover.Notifier.run()
>
> Chris
>
> -----Original Message-----
> From: Patricia Shanahan [mailto:pats@acm.org]
> Sent: Monday, April 04, 2011 7:20 AM
> To: dev@river.apache.org
> Subject: Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener
> can terminate discovery notifier threads
>
> I understand Tom's feeling against just logging, but I think it is
> probably the best option for now. Once we log, we should be able to find
>
> out if this is an issue, and if there are cases that are happening that
> would benefit from some other action.
>
> My really strong objection is to *silently* catching and carrying on.
> Partly, that is a result of having done a lot of debug, some of which
> was made unnecessarily difficult by software that destroyed clues.
>
> Patricia
>
>
> On 4/4/2011 2:15 AM, Tom Hobbs wrote:
>> You're right about InvocationHandler I should probably wake up before
> I send
>> emails.
>>
>> If the spec says that all "good" code throws ServerError we can leave
> that
>> Throwable catch in as well. This way we know that any of the latter
> means a
>> dos attack, non spec compliant services or something equally awful.
>>
>> I'm really reluctant to just leave a log and Throwable catch in; it
> just
>> feels wrong. I guess we might have to though since writing code for
> this
>> level requires a slightly different way of think than when at the
>> application level. I'm not going to keep flogging this dead horse
> though, I
>> trust your judgement on this more than mine. :-)
>>
>> Tom
>>
>> On 4 Apr 2011 09:44, "Dan Creswell"<da...@gmail.com> wrote:
>> Can't do anything about the Throwable as it's part of
> InvocationHandler and
>> that's the JDK spec.
>>
>> Could agree that our Dispatcher's only ever throw some specific
> subclasses.
>> We'd have to do some diligence on that as BasicInvocationDispatcher
> and
>> friends are designed to follow RMI spec, not entirely sure all other
>> transports can do enough in that respect to be compliant.
>>
>> There is one other problem with this however which is that badly
> written
>> service code could chuck out stuff that is not compliant and bring
> down the
>> entire house - that's kind of denial of service territory....
>>
>> ...personally I'd rather leave the catch throwable, log at some
> suitable
>> level and leave it at that, at least until we gather some data as to
> how
>> often this problem bites us etc.
>>
>>
>> On 4 April 2011 09:07, Tom Hobbs<tv...@googlemail.com> wrote:
>>
>>> Thanks for the info, Dan. Of c...
>>
>
>
RE: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can terminate discovery notifier threads
Posted by Christopher Dolan <ch...@avid.com>.
In this specific case, the Throwable is not relevant and not worth
logging because the locator is only used for a Level.FINEST log message!
More context from LookupLocatorDiscover.Notifier.run(), where the
catch(Throwable) is in the middle:
/* Log the event info about the lookup(s) */
if(firstListener && (logger.isLoggable(Level.FINEST)) ) {
String eType = (task.discard ?
"discarded":"discovered");
ServiceRegistrar[] regs = e.getRegistrars();
logger.finest(eType+" event -- "+regs.length
+" lookup(s)");
Map groupsMap = e.getGroups();
for(int i=0;i<regs.length;i++) {
LookupLocator loc = null;
try {
loc = regs[i].getLocator();
} catch (Throwable ex) { /* ignore */ }
String[] groups = (String[])groupsMap.get(regs[i]);
logger.finest(" "+eType+" locator = "+loc);
if(groups.length == 0) {
logger.finest(" "+eType
+" group = NO_GROUPS");
} else {
for(int j=0;j<groups.length;j++) {
logger.finest(" "+eType+" group["+j+"] "
+"= "+groups[j]);
}//end loop
}//endif(groups.length)
}//end loop
}//endif(firstListener && isLoggable(Level.FINEST)
There's nearly identical code in LookupDiscover.Notifier.run()
Chris
-----Original Message-----
From: Patricia Shanahan [mailto:pats@acm.org]
Sent: Monday, April 04, 2011 7:20 AM
To: dev@river.apache.org
Subject: Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener
can terminate discovery notifier threads
I understand Tom's feeling against just logging, but I think it is
probably the best option for now. Once we log, we should be able to find
out if this is an issue, and if there are cases that are happening that
would benefit from some other action.
My really strong objection is to *silently* catching and carrying on.
Partly, that is a result of having done a lot of debug, some of which
was made unnecessarily difficult by software that destroyed clues.
Patricia
On 4/4/2011 2:15 AM, Tom Hobbs wrote:
> You're right about InvocationHandler I should probably wake up before
I send
> emails.
>
> If the spec says that all "good" code throws ServerError we can leave
that
> Throwable catch in as well. This way we know that any of the latter
means a
> dos attack, non spec compliant services or something equally awful.
>
> I'm really reluctant to just leave a log and Throwable catch in; it
just
> feels wrong. I guess we might have to though since writing code for
this
> level requires a slightly different way of think than when at the
> application level. I'm not going to keep flogging this dead horse
though, I
> trust your judgement on this more than mine. :-)
>
> Tom
>
> On 4 Apr 2011 09:44, "Dan Creswell"<da...@gmail.com> wrote:
> Can't do anything about the Throwable as it's part of
InvocationHandler and
> that's the JDK spec.
>
> Could agree that our Dispatcher's only ever throw some specific
subclasses.
> We'd have to do some diligence on that as BasicInvocationDispatcher
and
> friends are designed to follow RMI spec, not entirely sure all other
> transports can do enough in that respect to be compliant.
>
> There is one other problem with this however which is that badly
written
> service code could chuck out stuff that is not compliant and bring
down the
> entire house - that's kind of denial of service territory....
>
> ...personally I'd rather leave the catch throwable, log at some
suitable
> level and leave it at that, at least until we gather some data as to
how
> often this problem bites us etc.
>
>
> On 4 April 2011 09:07, Tom Hobbs<tv...@googlemail.com> wrote:
>
>> Thanks for the info, Dan. Of c...
>
Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can
terminate discovery notifier threads
Posted by Patricia Shanahan <pa...@acm.org>.
I understand Tom's feeling against just logging, but I think it is
probably the best option for now. Once we log, we should be able to find
out if this is an issue, and if there are cases that are happening that
would benefit from some other action.
My really strong objection is to *silently* catching and carrying on.
Partly, that is a result of having done a lot of debug, some of which
was made unnecessarily difficult by software that destroyed clues.
Patricia
On 4/4/2011 2:15 AM, Tom Hobbs wrote:
> You're right about InvocationHandler I should probably wake up before I send
> emails.
>
> If the spec says that all "good" code throws ServerError we can leave that
> Throwable catch in as well. This way we know that any of the latter means a
> dos attack, non spec compliant services or something equally awful.
>
> I'm really reluctant to just leave a log and Throwable catch in; it just
> feels wrong. I guess we might have to though since writing code for this
> level requires a slightly different way of think than when at the
> application level. I'm not going to keep flogging this dead horse though, I
> trust your judgement on this more than mine. :-)
>
> Tom
>
> On 4 Apr 2011 09:44, "Dan Creswell"<da...@gmail.com> wrote:
> Can't do anything about the Throwable as it's part of InvocationHandler and
> that's the JDK spec.
>
> Could agree that our Dispatcher's only ever throw some specific subclasses.
> We'd have to do some diligence on that as BasicInvocationDispatcher and
> friends are designed to follow RMI spec, not entirely sure all other
> transports can do enough in that respect to be compliant.
>
> There is one other problem with this however which is that badly written
> service code could chuck out stuff that is not compliant and bring down the
> entire house - that's kind of denial of service territory....
>
> ...personally I'd rather leave the catch throwable, log at some suitable
> level and leave it at that, at least until we gather some data as to how
> often this problem bites us etc.
>
>
> On 4 April 2011 09:07, Tom Hobbs<tv...@googlemail.com> wrote:
>
>> Thanks for the info, Dan. Of c...
>
Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can
terminate discovery notifier threads
Posted by Dan Creswell <da...@gmail.com>.
:)
The horse ain't dead....
The trouble with this kind of stuff is there are many, many dark corners to
deal with. That's true of app code as well but perhaps it's less critical.
In this case I'd say catch throwable, catch RemoteException separately (if
you like, no objection here), a "loud" (WARN?) log-level which allows us to
figure out what goes on and how often. Once we know that we can tighten
policy if it makes sense.
On 4 April 2011 10:15, Tom Hobbs <tv...@googlemail.com> wrote:
> You're right about InvocationHandler I should probably wake up before I
> send
> emails.
>
> If the spec says that all "good" code throws ServerError we can leave that
> Throwable catch in as well. This way we know that any of the latter means
> a
> dos attack, non spec compliant services or something equally awful.
>
> I'm really reluctant to just leave a log and Throwable catch in; it just
> feels wrong. I guess we might have to though since writing code for this
> level requires a slightly different way of think than when at the
> application level. I'm not going to keep flogging this dead horse though, I
> trust your judgement on this more than mine. :-)
>
> Tom
>
> On 4 Apr 2011 09:44, "Dan Creswell" <da...@gmail.com> wrote:
> Can't do anything about the Throwable as it's part of InvocationHandler and
> that's the JDK spec.
>
> Could agree that our Dispatcher's only ever throw some specific subclasses.
> We'd have to do some diligence on that as BasicInvocationDispatcher and
> friends are designed to follow RMI spec, not entirely sure all other
> transports can do enough in that respect to be compliant.
>
> There is one other problem with this however which is that badly written
> service code could chuck out stuff that is not compliant and bring down the
> entire house - that's kind of denial of service territory....
>
> ...personally I'd rather leave the catch throwable, log at some suitable
> level and leave it at that, at least until we gather some data as to how
> often this problem bites us etc.
>
>
> On 4 April 2011 09:07, Tom Hobbs <tv...@googlemail.com> wrote:
>
> > Thanks for the info, Dan. Of c...
>
Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can
terminate discovery notifier threads
Posted by Tom Hobbs <tv...@googlemail.com>.
You're right about InvocationHandler I should probably wake up before I send
emails.
If the spec says that all "good" code throws ServerError we can leave that
Throwable catch in as well. This way we know that any of the latter means a
dos attack, non spec compliant services or something equally awful.
I'm really reluctant to just leave a log and Throwable catch in; it just
feels wrong. I guess we might have to though since writing code for this
level requires a slightly different way of think than when at the
application level. I'm not going to keep flogging this dead horse though, I
trust your judgement on this more than mine. :-)
Tom
On 4 Apr 2011 09:44, "Dan Creswell" <da...@gmail.com> wrote:
Can't do anything about the Throwable as it's part of InvocationHandler and
that's the JDK spec.
Could agree that our Dispatcher's only ever throw some specific subclasses.
We'd have to do some diligence on that as BasicInvocationDispatcher and
friends are designed to follow RMI spec, not entirely sure all other
transports can do enough in that respect to be compliant.
There is one other problem with this however which is that badly written
service code could chuck out stuff that is not compliant and bring down the
entire house - that's kind of denial of service territory....
...personally I'd rather leave the catch throwable, log at some suitable
level and leave it at that, at least until we gather some data as to how
often this problem bites us etc.
On 4 April 2011 09:07, Tom Hobbs <tv...@googlemail.com> wrote:
> Thanks for the info, Dan. Of c...
Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can
terminate discovery notifier threads
Posted by Dan Creswell <da...@gmail.com>.
Can't do anything about the Throwable as it's part of InvocationHandler and
that's the JDK spec.
Could agree that our Dispatcher's only ever throw some specific subclasses.
We'd have to do some diligence on that as BasicInvocationDispatcher and
friends are designed to follow RMI spec, not entirely sure all other
transports can do enough in that respect to be compliant.
There is one other problem with this however which is that badly written
service code could chuck out stuff that is not compliant and bring down the
entire house - that's kind of denial of service territory....
...personally I'd rather leave the catch throwable, log at some suitable
level and leave it at that, at least until we gather some data as to how
often this problem bites us etc.
On 4 April 2011 09:07, Tom Hobbs <tv...@googlemail.com> wrote:
> Thanks for the info, Dan. Of course the next most obvious question is, can
> we make this better?
>
> Can the throwable catch be replaced with an Exception and a ServerError
> catch. Of course this means making the docs explicit in that all remote
> stacks must behave as BasicInvocationDispatcher does and also changing the
> signature of InvocationHandler.invoke.
>
> Is it as easy as that, does anyone know? To my gut, being able to
> distinguish between a remote and a local critical problem would be a good
> thing.
>
> I'm just curious to see if rather than judging over things like this with
> log statements whether or not we can improve the behaviour.
>
> Cheers,
>
> Tom
>
> On 3 Apr 2011 21:19, "Dan Creswell" <da...@gmail.com> wrote:
> An Error gets wrapped into a ServerError which is a subclass of remote
> exception so long as you're using a BasicInvocationDispatcher (see the
> description for dispatch).
>
> For other remote stacks you mightn't get such behaviour and as far as I can
> see from a quick skim of the docs, it's not explicitly required to behave
> as
> BasicInvocationDispatcher does. Note also that InvocationHandler.invoke
> throws Throwable which allows the proxy of a remote service implementation
> to return anything it likes for any reason e.g. Error.
>
> Dan.
>
>
> On 3 April 2011 20:33, Tom Hobbs <tv...@googlemail.com> wrote:
>
> > I'm not saying you're wrong, I'...
>
Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can
terminate discovery notifier threads
Posted by Tom Hobbs <tv...@googlemail.com>.
Thanks for the info, Dan. Of course the next most obvious question is, can
we make this better?
Can the throwable catch be replaced with an Exception and a ServerError
catch. Of course this means making the docs explicit in that all remote
stacks must behave as BasicInvocationDispatcher does and also changing the
signature of InvocationHandler.invoke.
Is it as easy as that, does anyone know? To my gut, being able to
distinguish between a remote and a local critical problem would be a good
thing.
I'm just curious to see if rather than judging over things like this with
log statements whether or not we can improve the behaviour.
Cheers,
Tom
On 3 Apr 2011 21:19, "Dan Creswell" <da...@gmail.com> wrote:
An Error gets wrapped into a ServerError which is a subclass of remote
exception so long as you're using a BasicInvocationDispatcher (see the
description for dispatch).
For other remote stacks you mightn't get such behaviour and as far as I can
see from a quick skim of the docs, it's not explicitly required to behave as
BasicInvocationDispatcher does. Note also that InvocationHandler.invoke
throws Throwable which allows the proxy of a remote service implementation
to return anything it likes for any reason e.g. Error.
Dan.
On 3 April 2011 20:33, Tom Hobbs <tv...@googlemail.com> wrote:
> I'm not saying you're wrong, I'...
Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can
terminate discovery notifier threads
Posted by Dan Creswell <da...@gmail.com>.
An Error gets wrapped into a ServerError which is a subclass of remote
exception so long as you're using a BasicInvocationDispatcher (see the
description for dispatch).
For other remote stacks you mightn't get such behaviour and as far as I can
see from a quick skim of the docs, it's not explicitly required to behave as
BasicInvocationDispatcher does. Note also that InvocationHandler.invoke
throws Throwable which allows the proxy of a remote service implementation
to return anything it likes for any reason e.g. Error.
Dan.
On 3 April 2011 20:33, Tom Hobbs <tv...@googlemail.com> wrote:
> I'm not saying you're wrong, I'm saying I don't understand, especially
> given
> that I don't have the code in front of me. (actually, that's not true but
> a
> this tablet whatsit is awful for looking at code on).
>
> If I'm cailing a method on a proxy, the worst I can get is a remote
> exception. Right? How would a, for example, OutOfMemoryError thrown on a
> remote jvm find its way into my exception handling code without me
> rethrowing it? At best, wouldn't I only be able to see it via
> remoteException.getCause()?
>
> I've just never seen what you're describing. Only ever bad things in the
> re.getCause() method.
>
> Cheers,
>
> Tom
>
> On 3 Apr 2011 20:08, "Dan Creswell" <da...@gmail.com> wrote:
> Trouble is the Error could have come from a remote JVM, do you want that
> blowing up the local one?
>
>
> On 3 April 2011 19:08, Tom Hobbs <tv...@googlemail.com> wrote:
>
> > I think that exactly the argume...
>
Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can
terminate discovery notifier threads
Posted by Tom Hobbs <tv...@googlemail.com>.
I'm not saying you're wrong, I'm saying I don't understand, especially given
that I don't have the code in front of me. (actually, that's not true but a
this tablet whatsit is awful for looking at code on).
If I'm cailing a method on a proxy, the worst I can get is a remote
exception. Right? How would a, for example, OutOfMemoryError thrown on a
remote jvm find its way into my exception handling code without me
rethrowing it? At best, wouldn't I only be able to see it via
remoteException.getCause()?
I've just never seen what you're describing. Only ever bad things in the
re.getCause() method.
Cheers,
Tom
On 3 Apr 2011 20:08, "Dan Creswell" <da...@gmail.com> wrote:
Trouble is the Error could have come from a remote JVM, do you want that
blowing up the local one?
On 3 April 2011 19:08, Tom Hobbs <tv...@googlemail.com> wrote:
> I think that exactly the argume...
Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can
terminate discovery notifier threads
Posted by Dan Creswell <da...@gmail.com>.
Trouble is the Error could have come from a remote JVM, do you want that
blowing up the local one?
On 3 April 2011 19:08, Tom Hobbs <tv...@googlemail.com> wrote:
> I think that exactly the argument for not catching Throwable. In this case
> I'd catch Exception and log the message, thus allowing Error and other
> non-recoverable stuff to explode appropriately.
>
> If something really bad has happened let the code/user/whatever know.
> Don't
> confuse the issue by swallowing (or attempting to swallow) the problem.
>
> Just my opinion.
>
> Tom
>
> On 3 Apr 2011 16:05, "Dan Creswell" <da...@gmail.com> wrote:
> > On 3 April 2011 15:36, Patricia Shanahan <pa...@acm.org> wrote:
> >
> >> Correct about the class name.
> >>
> >> We need to remember that Throwable includes conditions that are not
> >> specific to the code that throws it, but rather indicate a general
> problem.
> >>
> >>
> > Sure but if you've got one of those cases, chances are many other things
> > will break also because it's a general problem.
> >
> > Patricia
> >>
> >>
> >>
> >> On 4/3/2011 1:14 AM, Dan Creswell wrote:
> >>
> >>> Did you mean LookupLocatorDiscovery.Notifier?
> >>>
> >>> In regards to ignoring Throwable, it depends on the context. In this
> case,
> >>> we'd have a ServiceRegistrar that cannot respond to the request made
> >>> (getLocator) which is therefore either down or pretty broken. The most
> I'd
> >>> do in such a situation is dump the registrar (but you might discover it
> >>> again later) and issue a log message.
> >>>
> >>> Silence in this case seems like a reasonable option, guess I might want
> a
> >>> log message to help me (or a user) debug is all.
> >>>
> >>> That just leaves whether one believes in catching Throwable. I'd say
> it's
> >>> legitimate as there's a variety of problems that could result from
> dealing
> >>> in a faulty ServiceRegistrar that shouldn't ultimately disrupt the
> >>> discovery
> >>> process.
> >>>
> >>> Cheers,
> >>>
> >>> Dan.
> >>>
> >>> On 3 April 2011 04:35, Patricia Shanahan<pa...@acm.org> wrote:
> >>>
> >>> In reviewing the code prior to applying the patch, I noticed the
> >>>> following in the LookupDiscoveryListener.Notifier.run() method:
> >>>>
> >>>> try {
> >>>> loc = regs[i].getLocator();
> >>>> } catch (Throwable ex) { /* ignore */ }
> >>>>
> >>>> What do people think about ignoring Throwable?
> >>>>
> >>>> Chris, Thanks for the patch. Do you happen to have a unit or QA test
> for
> >>>> this problem?
> >>>>
> >>>> Patricia
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> On 3/30/2011 3:05 PM, Chris Dolan (JIRA) wrote:
> >>>>
> >>>> Ill-behaved DiscoveryListener can terminate discovery notifier threads
> >>>>>
> ----------------------------------------------------------------------
> >>>>>
> >>>>> Key: RIVER-395
> >>>>> URL: https://issues.apache.org/jira/browse/RIVER-395
> >>>>> Project: River
> >>>>> Issue Type: Bug
> >>>>> Components: net_jini_discovery
> >>>>> Affects Versions: jtsk_2.1
> >>>>> Reporter: Chris Dolan
> >>>>>
> >>>>>
> >>>>> (bug detected in Jini 2.1, still present in 2.1.2+ trunk)
> >>>>>
> >>>>> If a net.jini.discovery.DiscoveryListener implementation throws an
> >>>>> unchecked exception, then the LookupLocatorDiscovery$Notifier thread
> >>>>> and/or
> >>>>> the LookupDiscovery$Notifier thread will exit prematurely. In
> practice,
> >>>>> this
> >>>>> can prevent the JoinManager$DiscMgrListener or
> >>>>> ServiceDiscoveryManager$DiscMgrListener callbacks from being invoked,
> >>>>> resulting in incomplete state for a registrar.
> >>>>>
> >>>>> A soon-to-be attached patch surrounds each listener invocation with a
> >>>>> try/catch block.
> >>>>>
> >>>>> --
> >>>>> This message is automatically generated by JIRA.
> >>>>> For more information on JIRA, see:
> >>>>> http://www.atlassian.com/software/jira
> >>>>>
> >>>>>
> >>>>>
> >>>>
> >>>
> >>
>
Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can
terminate discovery notifier threads
Posted by Tom Hobbs <tv...@googlemail.com>.
I think that exactly the argument for not catching Throwable. In this case
I'd catch Exception and log the message, thus allowing Error and other
non-recoverable stuff to explode appropriately.
If something really bad has happened let the code/user/whatever know. Don't
confuse the issue by swallowing (or attempting to swallow) the problem.
Just my opinion.
Tom
On 3 Apr 2011 16:05, "Dan Creswell" <da...@gmail.com> wrote:
> On 3 April 2011 15:36, Patricia Shanahan <pa...@acm.org> wrote:
>
>> Correct about the class name.
>>
>> We need to remember that Throwable includes conditions that are not
>> specific to the code that throws it, but rather indicate a general
problem.
>>
>>
> Sure but if you've got one of those cases, chances are many other things
> will break also because it's a general problem.
>
> Patricia
>>
>>
>>
>> On 4/3/2011 1:14 AM, Dan Creswell wrote:
>>
>>> Did you mean LookupLocatorDiscovery.Notifier?
>>>
>>> In regards to ignoring Throwable, it depends on the context. In this
case,
>>> we'd have a ServiceRegistrar that cannot respond to the request made
>>> (getLocator) which is therefore either down or pretty broken. The most
I'd
>>> do in such a situation is dump the registrar (but you might discover it
>>> again later) and issue a log message.
>>>
>>> Silence in this case seems like a reasonable option, guess I might want
a
>>> log message to help me (or a user) debug is all.
>>>
>>> That just leaves whether one believes in catching Throwable. I'd say
it's
>>> legitimate as there's a variety of problems that could result from
dealing
>>> in a faulty ServiceRegistrar that shouldn't ultimately disrupt the
>>> discovery
>>> process.
>>>
>>> Cheers,
>>>
>>> Dan.
>>>
>>> On 3 April 2011 04:35, Patricia Shanahan<pa...@acm.org> wrote:
>>>
>>> In reviewing the code prior to applying the patch, I noticed the
>>>> following in the LookupDiscoveryListener.Notifier.run() method:
>>>>
>>>> try {
>>>> loc = regs[i].getLocator();
>>>> } catch (Throwable ex) { /* ignore */ }
>>>>
>>>> What do people think about ignoring Throwable?
>>>>
>>>> Chris, Thanks for the patch. Do you happen to have a unit or QA test
for
>>>> this problem?
>>>>
>>>> Patricia
>>>>
>>>>
>>>>
>>>>
>>>> On 3/30/2011 3:05 PM, Chris Dolan (JIRA) wrote:
>>>>
>>>> Ill-behaved DiscoveryListener can terminate discovery notifier threads
>>>>> ----------------------------------------------------------------------
>>>>>
>>>>> Key: RIVER-395
>>>>> URL: https://issues.apache.org/jira/browse/RIVER-395
>>>>> Project: River
>>>>> Issue Type: Bug
>>>>> Components: net_jini_discovery
>>>>> Affects Versions: jtsk_2.1
>>>>> Reporter: Chris Dolan
>>>>>
>>>>>
>>>>> (bug detected in Jini 2.1, still present in 2.1.2+ trunk)
>>>>>
>>>>> If a net.jini.discovery.DiscoveryListener implementation throws an
>>>>> unchecked exception, then the LookupLocatorDiscovery$Notifier thread
>>>>> and/or
>>>>> the LookupDiscovery$Notifier thread will exit prematurely. In
practice,
>>>>> this
>>>>> can prevent the JoinManager$DiscMgrListener or
>>>>> ServiceDiscoveryManager$DiscMgrListener callbacks from being invoked,
>>>>> resulting in incomplete state for a registrar.
>>>>>
>>>>> A soon-to-be attached patch surrounds each listener invocation with a
>>>>> try/catch block.
>>>>>
>>>>> --
>>>>> This message is automatically generated by JIRA.
>>>>> For more information on JIRA, see:
>>>>> http://www.atlassian.com/software/jira
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can
terminate discovery notifier threads
Posted by Dan Creswell <da...@gmail.com>.
On 3 April 2011 15:36, Patricia Shanahan <pa...@acm.org> wrote:
> Correct about the class name.
>
> We need to remember that Throwable includes conditions that are not
> specific to the code that throws it, but rather indicate a general problem.
>
>
Sure but if you've got one of those cases, chances are many other things
will break also because it's a general problem.
Patricia
>
>
>
> On 4/3/2011 1:14 AM, Dan Creswell wrote:
>
>> Did you mean LookupLocatorDiscovery.Notifier?
>>
>> In regards to ignoring Throwable, it depends on the context. In this case,
>> we'd have a ServiceRegistrar that cannot respond to the request made
>> (getLocator) which is therefore either down or pretty broken. The most I'd
>> do in such a situation is dump the registrar (but you might discover it
>> again later) and issue a log message.
>>
>> Silence in this case seems like a reasonable option, guess I might want a
>> log message to help me (or a user) debug is all.
>>
>> That just leaves whether one believes in catching Throwable. I'd say it's
>> legitimate as there's a variety of problems that could result from dealing
>> in a faulty ServiceRegistrar that shouldn't ultimately disrupt the
>> discovery
>> process.
>>
>> Cheers,
>>
>> Dan.
>>
>> On 3 April 2011 04:35, Patricia Shanahan<pa...@acm.org> wrote:
>>
>> In reviewing the code prior to applying the patch, I noticed the
>>> following in the LookupDiscoveryListener.Notifier.run() method:
>>>
>>> try {
>>> loc = regs[i].getLocator();
>>> } catch (Throwable ex) { /* ignore */ }
>>>
>>> What do people think about ignoring Throwable?
>>>
>>> Chris, Thanks for the patch. Do you happen to have a unit or QA test for
>>> this problem?
>>>
>>> Patricia
>>>
>>>
>>>
>>>
>>> On 3/30/2011 3:05 PM, Chris Dolan (JIRA) wrote:
>>>
>>> Ill-behaved DiscoveryListener can terminate discovery notifier threads
>>>> ----------------------------------------------------------------------
>>>>
>>>> Key: RIVER-395
>>>> URL: https://issues.apache.org/jira/browse/RIVER-395
>>>> Project: River
>>>> Issue Type: Bug
>>>> Components: net_jini_discovery
>>>> Affects Versions: jtsk_2.1
>>>> Reporter: Chris Dolan
>>>>
>>>>
>>>> (bug detected in Jini 2.1, still present in 2.1.2+ trunk)
>>>>
>>>> If a net.jini.discovery.DiscoveryListener implementation throws an
>>>> unchecked exception, then the LookupLocatorDiscovery$Notifier thread
>>>> and/or
>>>> the LookupDiscovery$Notifier thread will exit prematurely. In practice,
>>>> this
>>>> can prevent the JoinManager$DiscMgrListener or
>>>> ServiceDiscoveryManager$DiscMgrListener callbacks from being invoked,
>>>> resulting in incomplete state for a registrar.
>>>>
>>>> A soon-to-be attached patch surrounds each listener invocation with a
>>>> try/catch block.
>>>>
>>>> --
>>>> This message is automatically generated by JIRA.
>>>> For more information on JIRA, see:
>>>> http://www.atlassian.com/software/jira
>>>>
>>>>
>>>>
>>>
>>
>
Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can
terminate discovery notifier threads
Posted by Patricia Shanahan <pa...@acm.org>.
Correct about the class name.
We need to remember that Throwable includes conditions that are not
specific to the code that throws it, but rather indicate a general problem.
Patricia
On 4/3/2011 1:14 AM, Dan Creswell wrote:
> Did you mean LookupLocatorDiscovery.Notifier?
>
> In regards to ignoring Throwable, it depends on the context. In this case,
> we'd have a ServiceRegistrar that cannot respond to the request made
> (getLocator) which is therefore either down or pretty broken. The most I'd
> do in such a situation is dump the registrar (but you might discover it
> again later) and issue a log message.
>
> Silence in this case seems like a reasonable option, guess I might want a
> log message to help me (or a user) debug is all.
>
> That just leaves whether one believes in catching Throwable. I'd say it's
> legitimate as there's a variety of problems that could result from dealing
> in a faulty ServiceRegistrar that shouldn't ultimately disrupt the discovery
> process.
>
> Cheers,
>
> Dan.
>
> On 3 April 2011 04:35, Patricia Shanahan<pa...@acm.org> wrote:
>
>> In reviewing the code prior to applying the patch, I noticed the
>> following in the LookupDiscoveryListener.Notifier.run() method:
>>
>> try {
>> loc = regs[i].getLocator();
>> } catch (Throwable ex) { /* ignore */ }
>>
>> What do people think about ignoring Throwable?
>>
>> Chris, Thanks for the patch. Do you happen to have a unit or QA test for
>> this problem?
>>
>> Patricia
>>
>>
>>
>>
>> On 3/30/2011 3:05 PM, Chris Dolan (JIRA) wrote:
>>
>>> Ill-behaved DiscoveryListener can terminate discovery notifier threads
>>> ----------------------------------------------------------------------
>>>
>>> Key: RIVER-395
>>> URL: https://issues.apache.org/jira/browse/RIVER-395
>>> Project: River
>>> Issue Type: Bug
>>> Components: net_jini_discovery
>>> Affects Versions: jtsk_2.1
>>> Reporter: Chris Dolan
>>>
>>>
>>> (bug detected in Jini 2.1, still present in 2.1.2+ trunk)
>>>
>>> If a net.jini.discovery.DiscoveryListener implementation throws an
>>> unchecked exception, then the LookupLocatorDiscovery$Notifier thread and/or
>>> the LookupDiscovery$Notifier thread will exit prematurely. In practice, this
>>> can prevent the JoinManager$DiscMgrListener or
>>> ServiceDiscoveryManager$DiscMgrListener callbacks from being invoked,
>>> resulting in incomplete state for a registrar.
>>>
>>> A soon-to-be attached patch surrounds each listener invocation with a
>>> try/catch block.
>>>
>>> --
>>> This message is automatically generated by JIRA.
>>> For more information on JIRA, see: http://www.atlassian.com/software/jira
>>>
>>>
>>
>
Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can terminate discovery notifier threads
Posted by Gregg Wonderly <gr...@gmail.com>.
I prefer to always log these things at a level below INFO so that the are nit invisible. I have been meaning to look over the code and actually create a be log level that is all of these kinds of things where no action is predescribed and we are ignoring something which might be attributed to a system config problem.
Gregg
Sent from my iPhone
On Apr 3, 2011, at 3:14 AM, Dan Creswell <da...@gmail.com> wrote:
> Did you mean LookupLocatorDiscovery.Notifier?
>
> In regards to ignoring Throwable, it depends on the context. In this case,
> we'd have a ServiceRegistrar that cannot respond to the request made
> (getLocator) which is therefore either down or pretty broken. The most I'd
> do in such a situation is dump the registrar (but you might discover it
> again later) and issue a log message.
>
> Silence in this case seems like a reasonable option, guess I might want a
> log message to help me (or a user) debug is all.
>
> That just leaves whether one believes in catching Throwable. I'd say it's
> legitimate as there's a variety of problems that could result from dealing
> in a faulty ServiceRegistrar that shouldn't ultimately disrupt the discovery
> process.
>
> Cheers,
>
> Dan.
>
> On 3 April 2011 04:35, Patricia Shanahan <pa...@acm.org> wrote:
>
>> In reviewing the code prior to applying the patch, I noticed the
>> following in the LookupDiscoveryListener.Notifier.run() method:
>>
>> try {
>> loc = regs[i].getLocator();
>> } catch (Throwable ex) { /* ignore */ }
>>
>> What do people think about ignoring Throwable?
>>
>> Chris, Thanks for the patch. Do you happen to have a unit or QA test for
>> this problem?
>>
>> Patricia
>>
>>
>>
>>
>> On 3/30/2011 3:05 PM, Chris Dolan (JIRA) wrote:
>>
>>> Ill-behaved DiscoveryListener can terminate discovery notifier threads
>>> ----------------------------------------------------------------------
>>>
>>> Key: RIVER-395
>>> URL: https://issues.apache.org/jira/browse/RIVER-395
>>> Project: River
>>> Issue Type: Bug
>>> Components: net_jini_discovery
>>> Affects Versions: jtsk_2.1
>>> Reporter: Chris Dolan
>>>
>>>
>>> (bug detected in Jini 2.1, still present in 2.1.2+ trunk)
>>>
>>> If a net.jini.discovery.DiscoveryListener implementation throws an
>>> unchecked exception, then the LookupLocatorDiscovery$Notifier thread and/or
>>> the LookupDiscovery$Notifier thread will exit prematurely. In practice, this
>>> can prevent the JoinManager$DiscMgrListener or
>>> ServiceDiscoveryManager$DiscMgrListener callbacks from being invoked,
>>> resulting in incomplete state for a registrar.
>>>
>>> A soon-to-be attached patch surrounds each listener invocation with a
>>> try/catch block.
>>>
>>> --
>>> This message is automatically generated by JIRA.
>>> For more information on JIRA, see: http://www.atlassian.com/software/jira
>>>
>>>
>>
Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can
terminate discovery notifier threads
Posted by Dan Creswell <da...@gmail.com>.
Did you mean LookupLocatorDiscovery.Notifier?
In regards to ignoring Throwable, it depends on the context. In this case,
we'd have a ServiceRegistrar that cannot respond to the request made
(getLocator) which is therefore either down or pretty broken. The most I'd
do in such a situation is dump the registrar (but you might discover it
again later) and issue a log message.
Silence in this case seems like a reasonable option, guess I might want a
log message to help me (or a user) debug is all.
That just leaves whether one believes in catching Throwable. I'd say it's
legitimate as there's a variety of problems that could result from dealing
in a faulty ServiceRegistrar that shouldn't ultimately disrupt the discovery
process.
Cheers,
Dan.
On 3 April 2011 04:35, Patricia Shanahan <pa...@acm.org> wrote:
> In reviewing the code prior to applying the patch, I noticed the
> following in the LookupDiscoveryListener.Notifier.run() method:
>
> try {
> loc = regs[i].getLocator();
> } catch (Throwable ex) { /* ignore */ }
>
> What do people think about ignoring Throwable?
>
> Chris, Thanks for the patch. Do you happen to have a unit or QA test for
> this problem?
>
> Patricia
>
>
>
>
> On 3/30/2011 3:05 PM, Chris Dolan (JIRA) wrote:
>
>> Ill-behaved DiscoveryListener can terminate discovery notifier threads
>> ----------------------------------------------------------------------
>>
>> Key: RIVER-395
>> URL: https://issues.apache.org/jira/browse/RIVER-395
>> Project: River
>> Issue Type: Bug
>> Components: net_jini_discovery
>> Affects Versions: jtsk_2.1
>> Reporter: Chris Dolan
>>
>>
>> (bug detected in Jini 2.1, still present in 2.1.2+ trunk)
>>
>> If a net.jini.discovery.DiscoveryListener implementation throws an
>> unchecked exception, then the LookupLocatorDiscovery$Notifier thread and/or
>> the LookupDiscovery$Notifier thread will exit prematurely. In practice, this
>> can prevent the JoinManager$DiscMgrListener or
>> ServiceDiscoveryManager$DiscMgrListener callbacks from being invoked,
>> resulting in incomplete state for a registrar.
>>
>> A soon-to-be attached patch surrounds each listener invocation with a
>> try/catch block.
>>
>> --
>> This message is automatically generated by JIRA.
>> For more information on JIRA, see: http://www.atlassian.com/software/jira
>>
>>
>