Skip to content

Commit

Permalink
Raise system error subclasses
Browse files Browse the repository at this point in the history
#41 added a translation of
`ECONNREFUSED` and `ECONNRESET` to `Trilogy::BaseConnectionError`.

We've got a few places at GitHub where we explicitly rescue these errors
(or `SystemCallError`). Although it is possible to rework those rescues
to check for particular error messages (like
trilogy-libraries/activerecord-trilogy-adapter@03c1610),
it'd be a bit easier if we could maintain compatibility. I also worry
that checking for specific error messages could be a bit brittle.

This commit introduces two new Trilogy errors, which inherit from the
corresponding `Errno` errors. We've already done something like this for
`Errno::ETIMEDOUT`.

This keeps Trilogy compatible with GitHub's existing rescues, while
still raising something that is a `Trilogy::Error` and a
`Trilogy::ConnectionError` (although NOT a
`Trilogy::BaseConnectionError` anymore).

A downside to this approach is that we might end up with a lot of
Trilogy-specific `Errno` subclasses. If that gets annoying perhaps we
could revisit when it comes time for a major release. Another approach
might be a single `Trilogy::SystemCallError` that inherits from
`SystemCallError` and then keeps a reference to the underlying `Errno`
error.
  • Loading branch information
composerinteralia committed Mar 7, 2023
1 parent 0776fcf commit a6b3765
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 22 deletions.
46 changes: 25 additions & 21 deletions contrib/ruby/ext/trilogy-ruby/cext.c
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@

VALUE Trilogy_CastError;
static VALUE Trilogy_BaseConnectionError, Trilogy_ProtocolError, Trilogy_SSLError, Trilogy_QueryError,
Trilogy_ConnectionClosedError, Trilogy_TimeoutError, Trilogy_Result;
Trilogy_ConnectionClosedError, Trilogy_ConnectionRefusedError, Trilogy_ConnectionResetError,
Trilogy_TimeoutError, Trilogy_Result;

static ID id_socket, id_host, id_port, id_username, id_password, id_found_rows, id_connect_timeout, id_read_timeout,
id_write_timeout, id_keepalive_enabled, id_keepalive_idle, id_keepalive_interval, id_keepalive_count,
Expand Down Expand Up @@ -78,6 +79,19 @@ static struct trilogy_ctx *get_open_ctx(VALUE obj)
return ctx;
}

NORETURN(static void trilogy_syserr_fail_str(int, VALUE));
static void trilogy_syserr_fail_str(int e, VALUE msg)
{
if (e == ECONNREFUSED) {
rb_raise(Trilogy_ConnectionRefusedError, "%" PRIsVALUE, msg);
} else if (e == ECONNRESET) {
rb_raise(Trilogy_ConnectionResetError, "%" PRIsVALUE, msg);
} else {
// TODO: All syserr should be wrapped.
rb_syserr_fail_str(e, msg);
}
}

NORETURN(static void handle_trilogy_error(struct trilogy_ctx *, int, const char *, ...));
static void handle_trilogy_error(struct trilogy_ctx *ctx, int rc, const char *msg, ...)
{
Expand All @@ -88,12 +102,7 @@ static void handle_trilogy_error(struct trilogy_ctx *ctx, int rc, const char *ms

switch (rc) {
case TRILOGY_SYSERR:
if (errno == ECONNREFUSED || errno == ECONNRESET) {
rb_raise(Trilogy_BaseConnectionError, "%" PRIsVALUE, rbmsg);
} else {
// TODO: All syserr should be wrapped.
rb_syserr_fail_str(errno, rbmsg);
}
trilogy_syserr_fail_str(errno, rbmsg);

case TRILOGY_ERR: {
VALUE message = rb_str_new(ctx->conn.error_message, ctx->conn.error_message_len);
Expand All @@ -106,13 +115,7 @@ static void handle_trilogy_error(struct trilogy_ctx *ctx, int rc, const char *ms
ERR_clear_error();
if (ERR_GET_LIB(ossl_error) == ERR_LIB_SYS) {
int err_reason = ERR_GET_REASON(ossl_error);

if (err_reason == ECONNREFUSED || err_reason == ECONNRESET) {
rb_raise(Trilogy_BaseConnectionError, "%" PRIsVALUE, rbmsg);
} else {
// TODO: All syserr should be wrapped.
rb_syserr_fail_str(err_reason, rbmsg);
}
trilogy_syserr_fail_str(err_reason, rbmsg);
}
// We can't recover from OpenSSL level errors if there's
// an active connection.
Expand Down Expand Up @@ -140,13 +143,8 @@ static VALUE allocate_trilogy(VALUE klass)
ctx->query_flags = TRILOGY_FLAGS_DEFAULT;

if (trilogy_init(&ctx->conn) < 0) {
if (errno == ECONNREFUSED || errno == ECONNRESET) {
rb_raise(Trilogy_BaseConnectionError, "trilogy_init");
} else {
// TODO: All syserr should be wrapped.
VALUE rbmsg = rb_str_new("trilogy_init", 13);
rb_syserr_fail_str(errno, rbmsg);
}
VALUE rbmsg = rb_str_new("trilogy_init", 13);
trilogy_syserr_fail_str(errno, rbmsg);
}

return obj;
Expand Down Expand Up @@ -1017,6 +1015,12 @@ void Init_cext()
Trilogy_TimeoutError = rb_const_get(Trilogy, rb_intern("TimeoutError"));
rb_global_variable(&Trilogy_TimeoutError);

Trilogy_ConnectionRefusedError = rb_const_get(Trilogy, rb_intern("ConnectionRefusedError"));
rb_global_variable(&Trilogy_ConnectionRefusedError);

Trilogy_ConnectionResetError = rb_const_get(Trilogy, rb_intern("ConnectionResetError"));
rb_global_variable(&Trilogy_ConnectionResetError);

Trilogy_BaseConnectionError = rb_const_get(Trilogy, rb_intern("BaseConnectionError"));
rb_global_variable(&Trilogy_BaseConnectionError);

Expand Down
8 changes: 8 additions & 0 deletions contrib/ruby/lib/trilogy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,14 @@ def initialize(error_message = nil, error_code = nil)
end
end

class ConnectionRefusedError < Errno::ECONNREFUSED
include ConnectionError
end

class ConnectionResetError < Errno::ECONNRESET
include ConnectionError
end

# DatabaseError was replaced by ProtocolError, but we'll keep it around as an
# ancestor of ProtocolError for compatibility reasons (e.g. so `rescue DatabaseError`
# still works. We can remove this class in the next major release.
Expand Down
2 changes: 1 addition & 1 deletion contrib/ruby/test/client_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def test_trilogy_connect_tcp_to_wrong_port
e = assert_raises Trilogy::ConnectionError do
new_tcp_client port: 13307
end
assert_equal "trilogy_connect - unable to connect to #{DEFAULT_HOST}:13307", e.message
assert_equal "Connection refused - trilogy_connect - unable to connect to #{DEFAULT_HOST}:13307", e.message
end

def test_trilogy_connect_unix_socket
Expand Down

0 comments on commit a6b3765

Please sign in to comment.