DRb rescue: Shouldn't this be changed?

[Mitch Mitch]

I'm a ruby newbie so I wanted to ask a forum about this before I emailed
the author. The current drb/observer.rb file consists of this code:

require 'observer'

module DRb
module DRbObservable
include Observable

def notify_observers(*arg)
if defined? @observer_state and @observer_state
if defined? @observer_peers
for i in @observer_peers.dup
begin
i.update(*arg)
rescue
delete_observer(i)
end
end
end
@observer_state = false
end
end
end
end

My issue is with the rescue. I after a bunch of time trying to track
down a bug I realized that I had a typo in an observer method which was
causing the observer to be deleted.

My suggestion is to change the line to:

rescue RangeError

Which only rescues the error that the author was intending (I believe).

Am I missing anything here?

 

[Chris Cummer]

My suggestion is to change the line to:

rescue RangeError

Which only rescues the error that the author was intending (I
believe).

Am I missing anything here?

RangeError is generally used only to catch errors with a numerical
range. I think what you're intending here is probably IndexError
however even that isn't correct since

for i in @observer_peers.dup
begin
i.update(*arg)
rescue
delete_observer(i)
end
end

since I don't believe a for...in loop can exceed the index in this
case unless something else is modifying @observer_peers at the same
time...?

I confess the "for...in" looks mighty strange to me, as I find
"@observer_peers.each do |i|" much easier to read these days.

I'd hazard a guess, from that little chunk of code, that the author's
intention was not to catch bounding errors with the loop but rather to
destroy any observers that were causing critical errors so as to not
have to deal with them. From the sounds of what you describe, it was
working as such as well, though silently destroying someone's stuff
seems just plain rude to me

These are all just guesses of course, as I've never used DRb and only
seen the chunk of code you posted. Hope that help.

Regards
Chris

 

[Mitch Mitch]

RangeError is generally used only to catch errors with a numerical

range. I think what you're intending here is probably IndexError
however even that isn't correct since


since I don't believe a for...in loop can exceed the index in this
case unless something else is modifying @observer_peers at the same
time...?

I've spent some quality time with DRb this last week. Before I
discovered drb/observer.rb I was trying to use the regular old
observer.rb. What would happen is an object would build up listeners
and, when a client would disconnect, I'd get a recycled object error.

I was trying to figure out how to get rid of those orphaned listeners.
I found drb/observer and all it did was add a "rescue" and
"delete_observer" in the notify_observers method, so I assumed this was
the reasoning behind it.

I believe that the RangeError is the correct exception for an object
recycled error (I found it in some random post). Please correct me if
I'm wrong.

I confess the "for...in" looks mighty strange to me, as I find
"@observer_peers.each do |i|" much easier to read these days.

I like a little "for...in" every now in then. It's more intuitive to me
than those crazy blocks. Still, I use "each" for Ruby solidarity

I'd hazard a guess, from that little chunk of code, that the author's
intention was not to catch bounding errors with the loop but rather to
destroy any observers that were causing critical errors so as to not
have to deal with them.

I'd say that's right. So, if I'm getting the author's intent right, a
little more specificity could save some headaches. Just hoping I could
add a contribution after struggling with this.

 

[Chris Cummer]

I've spent some quality time with DRb this last week. Before I
discovered drb/observer.rb I was trying to use the regular old
observer.rb. What would happen is an object would build up listeners
and, when a client would disconnect, I'd get a recycled object error.

I was trying to figure out how to get rid of those orphaned listeners.
I found drb/observer and all it did was add a "rescue" and
"delete_observer" in the notify_observers method, so I assumed this
was
the reasoning behind it.

I believe that the RangeError is the correct exception for an object
recycled error (I found it in some random post). Please correct me if
I'm wrong.

Ah I think I see now. Incidentally my knowledge of RangeError comes
largely from this post by Matz:

http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-talk/6576

however more googling indicates that your problem, and the use of
RangeError does seem to be correct. I now think I get why you were
doing:

rescue
delete_observer(i)
end

in there. That seems as reasonable patch as any to me to accomplish
what you're trying to do.

My gut tells me that ending up at that point with a RangeError due to
garbage collection issues per:

http://www.megasolutions.net/ruby/drb-recycled-object-problem-42669.aspx

might indicate a problem further up the program that might need more
detailing?

Hopefully someone with more knowledge of DRb and Ruby's GC will weigh
in and enlighten us both!

 

[Chris Cummer]

rescue
delete_observer(i)
end

in there. That seems as reasonable patch as any to me to accomplish
what you're trying to do.

That obviously should have been:

 

[Mitch Mitch]

however more googling indicates that your problem, and the use of

RangeError does seem to be correct. I now think I get why you were
doing:

rescue
delete_observer(i)
end

in there. That seems as reasonable patch as any to me to accomplish
what you're trying to do.

Thanks for the additional information.

I just want to make sure that I was clear. The code:
begin
...
rescue
delete_observer(i)
end

is not my patch, but the code in the current release of DRb
(drb/observer.rb). The patch I added in my code is:

begin
...
rescue RangeError
delete_observer(i)
end

 

[Eric Hodel]

[...]

My issue is with the rescue. I after a bunch of time trying to track
down a bug I realized that I had a typo in an observer method which
was
causing the observer to be deleted.

My suggestion is to change the line to:

rescue RangeError

Which only rescues the error that the author was intending (I
believe).

Am I missing anything here?

What about when the remote DRb service crashes?