Skip to content
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.

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

Fix parsing ERR after result set #158

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

clue
Copy link
Contributor

@clue clue commented Aug 20, 2022

This changeset fixes parsing an ERR response after a result set. Previously, this would trigger a parser error that would corrupt the entire connection ("Not enough data in buffer").

This error situation is a bit harder to reproduce. For example, this can be triggered in a connection by invoking a KILL QUERY n query from a second connection. Errors for invalid queries (SELECT unknown) would still trigger the existing logic.

In the following diagram, we already supported the transition from COM_QUERY_RESPONSE to ERR just fine. We now also support the transition from ROW/EOF to ERR:

graphviz-3ab2ba81081a7f3cc556d11fd09f50341bba6f15

From https://dev.mysql.com/doc/internals/en/com-query-response.html#text-resultset

Together with the analysis in ticket #123, you're looking at ~8 hours of work, enjoy!

Refs #123

@clue clue added the bug label Aug 20, 2022
@clue clue added this to the v0.5.7 milestone Aug 20, 2022
@clue clue requested a review from WyriHaximus August 20, 2022 16:55
@clue clue mentioned this pull request Aug 20, 2022
@clue
Copy link
Contributor Author

clue commented Aug 21, 2022

This error situation is a bit harder to reproduce. For example, this can be triggered in a connection by invoking a KILL QUERY n query from a second connection. Errors for invalid queries (SELECT unknown) would still trigger the existing logic.

This can also be reproduced with a simple EXPLAIN SELECT * FROM dual statement as per per https://dev.mysql.com/doc/internals/en/err-instead-of-eof.html. Any other SELECT 1 statement after this query would previously fail and now works as expected. The SELECT * FROM dual statement (without EXPLAIN) returns a proper error without a result set header, so any following statements already work in either case.

Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

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

👍, also merging it as the HHVM error is unrelated

@WyriHaximus WyriHaximus merged commit 3b6ab60 into friends-of-reactphp:0.5.x Aug 22, 2022
@clue clue deleted the resultset-err branch August 23, 2022 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants