Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicitly catch NoMethodError instead of catching with Object when there are no events in queue #215

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lamroger
Copy link

This can be replicated by loading a watch, stopping it, and removing it.

The logs look like this

I [2015-03-23 08:17:03]  INFO: simple-app-0 sent SIGKILL
I [2015-03-23 08:17:03]  INFO: simple-app-0 process stopped
I [2015-03-23 08:17:03]  INFO: simple-app-0 move 'up' to 'unmonitored'
I [2015-03-23 08:17:03]  INFO: simple-app-0 deregistered 'proc_exit' event for pid 4907
I [2015-03-23 08:17:03]  INFO: simple-app-0 moved 'up' to 'unmonitored'
F [2015-03-23 08:17:06] FATAL: Unhandled exception in driver loop - (NoMethodError): undefined         method `handle_event' for nil:NilClass
/var/lib/gems/1.9.1/gems/god-0.13.5/lib/god/driver.rb:181:in `block (2 levels) in initialize'
/var/lib/gems/1.9.1/gems/god-0.13.5/lib/god/driver.rb:179:in `loop'
/var/lib/gems/1.9.1/gems/god-0.13.5/lib/god/driver.rb:179:in `block in initialize'
I [2015-03-23 08:17:06]  INFO: simple-app-0 unwatched

The watch is a simple watch similar to the example on the homepage.

The test fails if you comment out the Object catch.

…le_event is called on nil

This occurs when a watch is stopped and then terminated.

Undo comments
@eric
Copy link
Collaborator

eric commented Mar 23, 2015

Instead of catching that exception, the code should handle the condition directly so it doesn't generate an exception.

I'm not sure if we should ever be seeing a nil in the event queue. Do you see how it gets there?

@lamroger
Copy link
Author

The events are stored in an array and are popped with the shift function.
The ruby array's pop function also returns nil if an array is empty so it
sounds reasonable.

I can modify it to check if it's nil before calling handle_event.

On Monday, March 23, 2015, Eric Lindvall notifications@github.com wrote:

Instead of catching that exception, the code should handle the condition
directly so it doesn't generate an exception.

I'm not sure if we should ever be seeing a nil in the event queue. Do you
see how it gets there?


Reply to this email directly or view it on GitHub
#215 (comment).

@eric
Copy link
Collaborator

eric commented Mar 24, 2015

It looks like the goal of DriverEventQueue#pop is to only return something valid.

I think that another raise ThreadError, "queue empty" if @shutdown needs to be added right before the @events.shift call.

@lamroger
Copy link
Author

I think the problem still occurs when DriverEventQueue#pop gets called and @shutdown is false. It never raises an exception.

I don't quite understand why we @resource.wait if it's not shutting down. Is it because we want to give our queue another chance to get populated?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants