Skip to content

Handle server disconnection #263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
byroot wants to merge 1 commit into
base: main
Choose a base branch
from
+28 −1
Open

Handle server disconnection #263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/macos.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ jobs:
name: Test (MariaDB ${{ matrix.mariadb }})
runs-on: macos-latest
strategy:
fail-fast: false
matrix:
mariadb: ["10.6", "10.11", "11.4", "11.8"]
steps:
Expand Down Expand Up @@ -127,6 +128,7 @@ jobs:
name: Test Ruby (MariaDB ${{ matrix.mariadb }}, Ruby ${{ matrix.ruby }})
runs-on: macos-latest
strategy:
fail-fast: false
matrix:
mariadb: ["10.6", "11.8"]
ruby: ["4.0"]
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).

### Fixed

- Handle server disconnects (Error 4031) correctly by raising `Trilogy::BaseConnectionError` instead of raising a QueryError / TRILOGY_INVALID_SEQUENCE_ID. #257.
- Column names in results are now encoded using the connection encoding. #210.

## 2.10.0
Expand Down
1 change: 1 addition & 0 deletions contrib/ruby/lib/trilogy/error.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ class ProtocolError < BaseError
1160 => BaseConnectionError, # ER_NET_ERROR_ON_WRITE
1161 => BaseConnectionError, # ER_NET_WRITE_INTERRUPTED
1927 => BaseConnectionError, # ER_CONNECTION_KILLED
4031 => BaseConnectionError, # Disconnected by server
}
class << self
def from_code(message, code)
Expand Down
23 changes: 23 additions & 0 deletions contrib/ruby/test/client_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1017,6 +1017,29 @@ def test_connection_invalid_dns
assert_includes ex.message, "getaddrinfo"
end

def test_session_wait_timeout
client = new_tcp_client
client.query("SET @@SESSION.WAIT_TIMEOUT=1;")
sleep(1.1)

if is_mariadb?
assert_raises Trilogy::SyscallError::ECONNRESET, Trilogy::EOFError do
client.query("SELECT 1")
end
else
ex = assert_raises Trilogy::BaseConnectionError do
client.query("SELECT 1")
end

assert_includes ex.message, "The client was disconnected by the server because of inactivity"
assert_equal 4031, ex.error_code
end

assert_raises Trilogy::EOFError do
client.query("SELECT 1")
end
Comment on lines +1034 to +1040
Copy link
Contributor

@HeyNonster HeyNonster Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@composerinteralia if the server is sending back a 4031 here should Trilogy know the connection is closed and then return a ConnectionClosed error instead of an EOFError?

Copy link
Collaborator Author

@byroot byroot Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally yes, in practice it's way trickier to implement than it sounds.

Copy link
Contributor

@HeyNonster HeyNonster Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been trying to find any mysql documentation that details all of the possible out-of-sequence messages and I've come up blank. 😞

Copy link
Collaborator

@composerinteralia composerinteralia Feb 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that it's ideal but probably not so easy to do. It's also a bit awkward that trilogy_error_recoverable_p will return true for this case, but we'd find out quickly on the next attempt that the connection is closed, so 🤷🏻.

I guess the only worry would be if there are any server-initiated communications that don't also close the connection immediately. That'd leave us in a bad state. But it might not be possible. I'm not too sure.

Copy link
Contributor

@HeyNonster HeyNonster Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any server-initiated communications that don't also close the connection immediately

Yeah, this is what I was trying to find documentation for but found nada 🤷

Copy link

@Thomascountz Thomascountz Feb 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In libmysqlclient, when it expects a pkt_nr of 1, but it gets 0, it marks the socket as unusable and proceeds with parsing, since "The caller should handle the error code in the packet..."

https://github.com/mysql/mysql-server/blob/447eb26e094b444a88c532028647e48228c3c04f/sql-common/net_serv.cc#L1518-L1560

The logic is:

  1. If the sequence isn't what we expect: if (pkt_nr != (uchar)net->pkt_nr)
  2. And what we expected was a 1: if (net->pkt_nr == 1)
  3. Mark the socket as permanently dead: net->error = NET_ERROR_SOCKET_UNUSABLE;
  4. Record that we detected an out-of-order packet: net->last_errno = ER_NET_PACKETS_OUT_OF_ORDER;
  5. Update our expected sequence number to be 1: net->pkt_nr = pkt_nr + 1;
  6. Return false meaning "no error reading the header" and continue parsing: return false

So, I'm thinking we can do the same. We can assume the socket is closed in this specific case (expecting 1, got 0) and continue parsing/handling the error message. I think it's possible there are other 0-sequence out-of-order interruptions from the server, but this is the only "documented" case I could find.

end

def test_memsize
require 'objspace'
client = new_tcp_client
Expand Down
2 changes: 1 addition & 1 deletion src/packet_parser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ size_t trilogy_packet_parser_execute(trilogy_packet_parser_t *parser, const uint
break;
}
case S_SEQ: {
if (cur_byte != parser->sequence_number) {
if (cur_byte != parser->sequence_number && cur_byte > 0) {
Copy link
Collaborator

@composerinteralia composerinteralia Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some vague concerns that this could mask some legitimate error case, but I couldn't think of any such cases so maybe it's fine.

Copy link
Collaborator Author

@byroot byroot Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it felt a bit bad writtting this, but I have no better idea.

*error = TRILOGY_INVALID_SEQUENCE_ID;
return i;
}
Expand Down