river-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dan Creswell <dan.cresw...@gmail.com>
Subject Re: [jira] [Created] (RIVER-395) Ill-behaved DiscoveryListener can terminate discovery notifier threads
Date Tue, 05 Apr 2011 07:39:47 GMT
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 <pats@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"<dan.creswell@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<tvhobbs@googlemail.com>   wrote:
>>>
>>>  Thanks for the info, Dan. Of c...
>>>>
>>>
>>>
>>
>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message