Error class refactoring (part 2)

[adrianna-chang-shopify]

Follow up to: #15

Converts a few more Trilogy errors:

• TRILOGY_DNS_ERROR is generally returned when we attempt to connect to a db with an unknown hostname, which we should treat as a connection error and handle properly in the adapter.
• Trilogy::ConnectionClosed was introduced in the last PR to handle IOError, but I forgot to actually raise the Trilogy error in the C ext, so that's been updated
• Handles known syscall errors: ECONNREFUSED and ECONNRESET, which should both be handled as connection errors. These are the ones I've been encountering frequently; we can expand on the error conversions as we see more pop up. I'm not sure how many of the 107 syscall errors we actually expect to see.

I plan to update trilogy-libraries/activerecord-trilogy-adapter#24 to handle these changes.

[paarthmadan]

🎉

[jhawthorn]

I should have given this feedback in the previous PR 😓, but should we rename this ConnectionError? I find the "Base" prefix a little out of place as nothing inherits this.

[adrianna-chang-shopify]

👋 Hey @jhawthorn ! Yeah unfortunately the naming is a little weird at the moment because we're handling some of the new error classification via modules to preserve backwards compatibility. (See "Backward compatibility" in the original issue).

We currently have ConnectionError defined as a module, so that consumers could do rescue Trilogy::ConnectionError to rescue connection-related errors, while existing code could retain compatibility if they're doing rescue IOError, etc.

Is there a better name we could use for the class itself, other than BaseConnectionError? Or perhaps we ought to be sticking with a better naming convention, foregoing backwards compatibility, and working towards a v3.0 instead with these changes?

[adrianna-chang-shopify]

Still seeing pretty consistent failures for test_connection_error with MySQL 8.0 🤔 I haven't been able to repro locally though. Any progress on that end @composerinteralia ?

[composerinteralia]

I can get failures running via the CI docker files:

MYSQL_VERSION=8 DOCKERFILE=Dockerfile.test.buster script/cibuild

But it doesn't fail consistently even with the same seed. I've seen failures running just that one test, but seems to fail more often when I run the full suite.

I also tried running in a loop and it seems to either fail on the first iteration or succeed for the whole loop:

  def test_connection_error
    1000.times do |i|
      p i
      Trilogy.new(
        :host => "db.local",
        :port => 3306,
        :username => "foo",
        :password => nil,
        :ssl => true,
        :ssl_mode => 4,
        :tls_min_version => 3
      ).close
    rescue Trilogy::BaseConnectionError => e
    end
  end

Same story on main before the error changes.

It looks like when the test passes we read an err packet, and when it fails we read an auth switch packet, so that's something: https://github.com/github/trilogy/blob/10aee3ba880af40658ad19d3a9ff09a57adada01/src/client.c#L432-L440

I think we can remove this test for now and add it back once we've addressed the flakiness.

[composerinteralia]

Or maybe this? #42

[adrianna-chang-shopify]

@composerinteralia @jhawthorn would love to get your help to move this along!

• For now, I've skipped the test_connection_error (with a note linking to Avoid protocol error on empty password auth switch #42, which is where most of the context is tracked so far) until we have a chance to investigate this further. I'm wondering if it could possibly be OS related.. I'm on arm64-darwin21 and still can't replicate the failures on my end, whereas CI is x86_64-darwin21 (not sure what your machine is, Daniel).
• @jhawthorn I know you had some concerns around the naming in Error class refactoring (part 2) #41 (comment), what are your thoughts on the current "module-approach" to preserve backwards compat?
• One thing I noticed and could use feedback on: TRILOGY_CLOSED_CONNECTION is currently surfaced as a QueryError, and that seems potentially incorrect to me. Do we think this should be a Trilogy::ConnectionClosed error?

Let me know if there's anything else you'd like me to address 😄