Skip to content

Return false instead of raising an error when connection is lost in Trilogy#ping method#145

Closed
genya0407 wants to merge 2 commits into
trilogy-libraries:mainfrom
genya0407:main
Closed

Return false instead of raising an error when connection is lost in Trilogy#ping method#145
genya0407 wants to merge 2 commits into
trilogy-libraries:mainfrom
genya0407:main

Conversation

@genya0407
Copy link
Copy Markdown
Contributor

This pull request modifies the behavior of the Trilogy#ping method. Now, it returns false instead of raising an error when the connection is lost. This change will help users handle connection loss more gracefully and avoid unnecessary exceptions.

The test case test_trilogy_ping_after_close_returns_false suggests the original intention was to return false when the connection is lost.
Additionally, Mysql2::Client#ping returns false instead of raising an error when the connection is lost. If Trilogy adopts the same behavior, it would make it easier for users to switch between the two libraries.

Furthermore, this change is expected to work without any issues in the trilogy_adapter of Rails, as Trilogy#ping method is only used in the following method:
https://github.com/rails/rails/blob/dca72dab117e79ee75666927935a7a6a31dd5324/activerecord/lib/active_record/connection_adapters/trilogy_adapter.rb#L120-L124

@genya0407 genya0407 marked this pull request as ready for review January 1, 2024 12:32
@jhawthorn
Copy link
Copy Markdown
Collaborator

I don't think we should do this. As was discussed in #134 we deliver more information to the user by raising exceptions. It also allows potentially reusing the same error handling as we do for queries and other operations.

It's also not good for us here to return false without permanently closing the connection, as that could lead to an inconsistent state if further attempts are made to use it.

@genya0407
Copy link
Copy Markdown
Contributor Author

Thank you for your review. I’ve looked over the discussion in #134.

I concur that if the ping method raises an error, it can provide more detailed information to the user and ensure the connection is closed through existing exception handling. Upon further reflection, this proposed change might not be ideal, as any operation that relies on Trilogy#ping raising an error would not be able to handle a connection loss correctly.

In our specific use case, we have verified that the system functions correctly even when the ping method raises an error. So, I am comfortable with closing this PR. However, if many users are looking to transition from mysql2 to Trilogy but are hindered by the behavior of the ping method, it might be worthwhile to revisit this behavior change.

Additionally, it seems more accurate to rename the test case from test_trilogy_ping_after_close_returns_false to test_trilogy_ping_after_close_raises. If you agree, I will create a separate PR to implement this correction.

Thank you again for your time and consideration.

@jhawthorn
Copy link
Copy Markdown
Collaborator

Yes, I think that test name is just wrong (and as far as I can tell is as old as this project). A PR would be appreciated 😊

Though Trilogy is very similar to mysql2 (in part likely due to shared authorship) they aren't meant to be 1:1 drop-in compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants