Raise a ruby exception for InvalidWatchSignature #757
Conversation
|
Not sure why the travis build failed, didn't seem like a related failure. If it is related, can you please help me with fixing it? |
|
The build failure was unrelated, I just re-ran the build so you can see your code turns up green. This looks good! I have a suggestion/request for a change: The signatures system is used by more than just file watching, so I think the exception should be called |
|
Thanks for getting back quickly! Made the refactor and added a unit test case as well 👍 |
| file = Tempfile.new('foo') | ||
|
|
||
| w1 = EventMachine.watch_file(file.path) | ||
| w2 = EventMachine.watch_file(file.path) |
There was a problem hiding this comment.
This returns a real Watcher object that isn't valid? That seems bad, like, the better solution here is to return nil if the file is already being watched and we can't watch it a second time.
In the case of kqueue, does it actually create two independent watchers?
There was a problem hiding this comment.
The watcher it returns is valid but the binding to watch map that is used to keep reference of the watch is not properly updated, so stopping the watch raises this error.
The inotify and kqueue behaviour is reported more thoroughly in #512.
| evma_unwatch_filename(NUM2BSIG (sig)); | ||
| } catch (std::runtime_error e) { | ||
| rb_raise (EM_eInvalidSignature, "%s", e.what()); | ||
| } |
There was a problem hiding this comment.
I agree with this regardless of the reason for the invalid signature; raising a C++ exception into Ruby space that kills the application isn't helpful to anybody.
Addresses #512
Some how the runtime error thrown in evma_unwatch_filename was not caught and EM crashed with std::runtime_error. A Simple patch to fix this by throwing a ruby exception instead.
Not sure if this is the correct fix but at least the ruby exception can be caught and handled further.