Conversation
byroot
force-pushed
the
timeout-desync
branch
2 times, most recently
from
56c8ecc to
945cfb1
Compare
| return i; | ||
| } | ||
|
|
||
| parser->sequence_number++; |
There was a problem hiding this comment.
I think this is good! I have one question though:
Is there any scenario where we would want to continue reading responses after getting a 0 response? I assume any 0 response that's unexpected will be an error/closed connection but I'm not positive.
5 0 Server Greeting proto=10 version=8.0.44
7 1 Login Request user=root
9 2 Response OK
11 0 Request Query
13 1 Response OK
15 0 Response Error 4031 <-- end here, connection closed.
19 0 Request Query
Because I think if we increment here then the parser would next expect a sequence number of 2 but we haven't gotten 1 back yet. It's fine in this scenario because we'll never get a 1 back anyway.
It's likely I'm overthinking this.
There was a problem hiding this comment.
No, you're not wrong. I think my test should at least assert the state of the connection after the error.
There was a problem hiding this comment.
Hum. I added a test, but the behavior seem quite inconsistent across mysql implementations and platforms :/
byroot
force-pushed
the
timeout-desync
branch
from
945cfb1 to
466241f
Compare
byroot
force-pushed
the
timeout-desync
branch
from
466241f to
75ad9b4
Compare
Fix: trilogy-libraries#257 When the server triggers a disconnection, we receive a packet with sequence 0. Hence we can't be so strict about sequence ordering.
byroot
force-pushed
the
timeout-desync
branch
from
75ad9b4 to
bcca0f3
Compare
| 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 |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
Ideally yes, in practice it's way trickier to implement than it sounds.
There was a problem hiding this comment.
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. 😞
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤷
There was a problem hiding this comment.
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..."
The logic is:
- If the sequence isn't what we expect:
if (pkt_nr != (uchar)net->pkt_nr) - And what we expected was a
1:if (net->pkt_nr == 1) - Mark the socket as permanently dead:
net->error = NET_ERROR_SOCKET_UNUSABLE; - Record that we detected an out-of-order packet:
net->last_errno = ER_NET_PACKETS_OUT_OF_ORDER; - Update our expected sequence number to be
1:net->pkt_nr = pkt_nr + 1; - 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.
| } | ||
| case S_SEQ: { | ||
| if (cur_byte != parser->sequence_number) { | ||
| if (cur_byte != parser->sequence_number && cur_byte > 0) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, it felt a bit bad writtting this, but I have no better idea.
4 participants
Fix: #257
When the server triggers a disconnection, we receive a packet with sequence 0. Hence we can't be so strict about sequence ordering.