trilogy-libraries / activerecord-trilogy-adapter

Active Record adapter for the Trilogy database client for Rails v6.0 - v7.0.
https://github.com/trilogy-libraries/trilogy
MIT License
172 stars 17 forks source link

Regression: Trilogy::SyscallError::EADDRNOTAVAIL no longer converted to ActiveRecord::StatementInvalid #52

Closed dhruvCW closed 1 year ago

dhruvCW commented 1 year ago

In our CI when we run

#!/bin/bash -eo pipefail
if (egrep '^  grape($|\s)' Gemfile.lock > /dev/null) ; then
  bundle exec rails runner 'puts API::Base.routes.map(&:path)' > grape-routes.txt
fi

we get the following error message (since trilogy adapter 3.1.0)

 /home/circleci/project/vendor/bundle/ruby/3.2.0/gems/trilogy-2.4.1/lib/trilogy.rb:174:in `_initialize': Cannot assign requested address - trilogy_connect - unable to connect to localhost:3306 (Trilogy::SyscallError::EADDRNOTAVAIL)

this is a regression since previously it used to result in

Failed to validate the schema cache because of ActiveRecord::StatementInvalid: Trilogy::SyscallError::EADDRNOTAVAIL: Cannot assign requested address - trilogy_connect - unable to connect to localhost:3306
Failed to validate the schema cache because of ActiveRecord::StatementInvalid: Trilogy::SyscallError::EADDRNOTAVAIL: Cannot assign requested address - trilogy_connect - unable to connect to localhost:3306

without any breaks.

dhruvCW commented 1 year ago

fixed in https://github.com/trilogy-libraries/trilogy/pull/98

dbussink commented 1 year ago

What does this behave like on Rails main itself with Trilogy? https://github.com/trilogy-libraries/activerecord-trilogy-adapter/pull/49 removed the translation since that broke other things and matches the upstream Rails code more closely. Does Rails main fail with the same error as you show here as well?

dhruvCW commented 1 year ago

Haven't tested this on rails main but this isn't actually a problem the adapter should solve. The client should be returning this as a Trilogy::BaseConnectionError which is then translated by the adapter as a ActiveRecord::DatabaseConnectionError. I believe #49 just exposed this problem.

with trilogy-libraries/trilogy#98 the behaviour is now the same as with mysql2.

lorint commented 1 year ago

@dhruvCW, believe I'd now like to know more about this perfect situation.

dhruvCW commented 1 year ago

@dhruvCW, believe I'd now like to know more about this perfect situation.

If you are refering to the situation that triggers this error. It seems to exclusively affect our usage of circle-ci. We have two parallel jobs one that runs rspec against a test database as a CI dependency (splitting the tests across multiple jobs), and a job that run rspec + the above mentioned route dumps. The latter is the point where we face this problem. Based on a bit of google-fu it seems that the client cannot connect to the port/host combination (localhost) because another entity is already bound to it. This might make sense if we have multiple processes accessing the port but I am not sure my reasoning is right 🤷

as for the solution, handling this as a Trilogy::BaseConnectionError with the text DNS_ERROR in the client initialization code allows us (the adapter) to handle this as a DBConnectionError for the hostname which IMO is the correct solution since that is also the result in this situation with mysql2.

lorint commented 1 year ago

the client cannot connect to the port/host combination (localhost) because another entity is already bound to it

Seems curious only because MySQL should allow for 150 or 200 connections. You can see how many your instance has set by running:

mysql -uroot --execute='SHOW VARIABLES LIKE "max_connections";' 
dhruvCW commented 1 year ago

Seems curious only because MySQL should allow for 150 or 200 connections. You can see how many your instance has set by running: mysql -uroot --execute='SHOW VARIABLES LIKE "max_connections";'

my apologies for not being clear we don't have a running mysql server for the job that fails (the socket error seems to be for the client itself).

lorint commented 1 year ago

Ah, cool.

As a side-note, there is a PR for edge Rails that is likely to drop soon which affects Trilogy error messages. It does not look like it will impact this issue, but I wanted to examine this as we move forward with a fix.

dhruvCW commented 1 year ago

As a side-note, there is a https://github.com/rails/rails/pull/48109. It does not look like it will impact this issue, but I wanted to examine this as we move forward with a fix.

thanks for sharing this PR based on the proposed changes this situation should result in a ConnectionFailed error (since on trilogy main all exceptions are derived from Trilogy::Error). so we should be fine 👍

dhruvCW commented 1 year ago

closing in favour of https://github.com/trilogy-libraries/trilogy/issues/113