Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Create EM exceptions namespaces #1

Open
ibc opened this Issue · 16 comments

2 participants

@ibc
Owner

Lot of C++ code in EM creates exceptions uncaught at Ruby level. They should finally to rb_raise with some appropriate EM exception class. For example:

 } catch (std::runtime_error e) {
     rb_raise (EM_eConnectionError, e.what());
 }
@ibc ibc was assigned
@ibc
Owner

This work should be done in the new "cleanup" branch.

@ibc
Owner

eventmachine#225 (Changed _Run*Once() to be void, always returned true) added to "cleanup" branch.

@ibc
Owner

About the C++ exceptions, I don't want all of them to become a Ruby exception that can be "rescued". I do understand that this is complex C++ code and in case there is a fatal bug (due to a C++ bug) the best option is to abruptly exit the program, rather than creating a Ruby exception. If a Ruby exception is created it could be rescued by the user and perhaps a C++ memory leak could occur (or anything).

So I just want to handle those C++ exceptions that are safe to convert into Ruby exceptions.

The EM.set_uid is a good example. IMHO it would be better that it creates some EM::XxxxError (to be defined). Of course, the user decides what to do then (it would be annoying if it just rescues and ignores the exception and leaves the program to run as root, but that's is the developer's responsability). I'm not sure of which other C++ exceptions could be migrated to Ruby exceptions.

@ibc
Owner

There are cases, yes, for example:


extern "C" int evma_detach_fd (const unsigned long binding)
{
ensure_eventmachine("evma_detach_fd");
EventableDescriptor ed = dynamic_cast <EventableDescriptor> (Bindable_t::GetObject (binding));
if (ed)
return EventMachine->DetachFD (ed);
else
#ifdef BUILD_FOR_RUBY
rb_raise(rb_eRuntimeError, "invalid binding to detach");
#else
throw std::runtime_error ("invalid binding to detach");
#endif
return -1;
}

In this case:

  • The "#ifdef BUILD_FOR_RUBY" is useless given the fact that cmain.cpp is always compiled for Ruby (yes, theorically it can be used in pure C++ but, does somebody really use it or has tested it?).
  • Raising RuntimeError instead of a custom EM::XxxxError is just bad.
@ibc
Owner

There is a pull request trying to offer exactly this stuff:
eventmachine#204

@cabo
Collaborator

Calling rb_raise from C++ code may skip the destruction of automatic variables.

See

http://old.thoughtsincomputation.com/posts/ruby-c-extensions-c-and-weird-crashing-on-rb_raise

and

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

In some cursory examination of the C++ code, I have not found any automatic variables with non-trivial destructors, but

1) I could have overlooked some

2) a future change might introduce some.

From complex C++ code I would throw C++ exceptions and catch them at the Ruby interface level, converting them to Ruby exceptions in a very simple function that just obviously is not using any C++ destructor magic.
This kind of catching/converting is already done in 6 places in rubymain.cpp.
We just need to find the missing ones.
We also could do away with some exceptions that really just mirror return codes for other error conditions.

@ibc
Owner

It's fully clear now. Could be possible for you to add some love to this issue? my C++ knowledge is null.

IMHO after this issue is "fixed" and we merge it into master branch, we can release the first "stable" Gem version 1.1.0 and announce it into eventmachine maillist, do you agree?

@cabo
Collaborator
@cabo
Collaborator
@ibc
Owner

About pull request 300, it is in master: 9ea2829

About run_once fix, I included it in "cleanup" branch (it's minimal).

I don't see more interesting pull requests for now. There are some ones that I've not checked fully yet.

I will reply to your previous comment soon.

@ibc
Owner

About your previous comment:

  • OK, I will create test units for those cases in which a Ruby exception is desirable.

  • Ok with test_ipv6_tcp_client_with_ipv6_google_com. In fact that test some times fails in my host (maybe due to my IPv6 over Teredo/Miredo). I will rename it (already done in fact).

  • "The test is also brittle with the hard-coded IPv6 address". Yes, but it you set the domain, EM resolver will just look for IPv4, and I don't want to add em-udns dependency to the test unit ;)

@ibc
Owner

One comment more: I have my SIP proxy which since yesterday it depends on eventmachine-le gem ;)
It's running in my server and it's used daily by some people which is testing it at SIP level. I inspect logs daily so I would realize of issues. It's ok to have it running some days before publishing it.

@cabo
Collaborator
@cabo
Collaborator
@ibc
Owner

1) Houston we have a problem. I didn't realize that 1.8.7 has no Addrinfo, so some tests will fail (test_ipv4, text_ipv6 and test_epoll which make usage of my new helpers in em_test_helper) !!!
I will open an issue for this.

2) I don't know more poeplo which can be implicated, but for sure I will do a heavy test of my server running this Gem since it's an active project in which I work wich other person (he does the JavaScript part however which speaks WebSocket with my server).

@ibc ibc referenced this issue from a commit
@ibc Added a test unit which uses EM.set_effective_user("non_existing_user…
…") and expects ArgumentError, but of course EM raises a C++ exception and exits (related to issue #1).
530184d
@ibc
Owner

test_no_cpp_exceptions.rb added. For now it includes a single test unit using EM.set_effective_user("non_existing_user") and expecting a Ruby ArgumentError, but currently EM creates a C++ exception and exits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.