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 some error processing bugs in driver, please read the commit message and follow! #127

Merged
merged 3 commits into from Feb 22, 2018

Conversation

Projects
None yet
3 participants
@SandorDobi
Copy link
Contributor

SandorDobi commented Feb 21, 2018

The driver several times improperly implementing the MySQL Client-Server protocol

Generally the ERR_Packet parsing is what missed.
If the server sending an ERR_Packet it was misinterpreted or not interpreted at all
Misinterpretation was in connection handshake task, when the server sent a ERR_packet
with Too many connections error, it was interpreted as handshake error and misleading the
developer

Silencing an error much more dangerous.
For example the PrepareQuery task can hang the server thread if sends to the server a
wrong SQL query.
The server will respond with an ERR_Packet which was simply ignored.
This method have a closure which must be called with a preprared statement for binding
The error condition must be delivered to this closure to be able to fail the promise

Other places was missing error processing as well

TODO:
Implement the COM_STMT_FETCH command correctly rather than blaming the MYSQL protocol
Refactor would be welcome, to implement in every Task (workflow) the correct packet flows
and error handling
Implement the missing Tasks:
- COM_STMT_PING for connection keep alive
- COM_STMT_BULK_EXECUTE for Bulk insert
- COM_STMT_SEND_LONG_DATA for big data Insert/Update
- COM_STMT_QUERY can use LOCAL INFILE syntax for File uploading. Implement this feature

The driver several times improperly implementing the MySQL Client-Ser…
…ver protocol

Generally the ERR_Packet parsing is what missed.
If the server sending an ERR_Packet it was misinterpreted or not interpreted at all
Misinterpretation was in connection handshake task, when the server sent a ERR_packet
with `Too many connections` error, it was interpreted as handshake error and misleading the
developer

Silencing an error much more dangerous.
For example the PrepareQuery task can hang the server thread if sends to the server a
wrong SQL query.
The server will respond with an ERR_Packet which was simply ignored.
This method have a closure which must be called with a preprared statement for binding
The error condition must be delivered to this closure to be able to fail the promise

Other places was missing error processing as well

TODO:
   Implement the COM_STMT_FETCH command correctly rather than blaming the MYSQL protocol
   Refactor would be welcome, to implement in every Task (workflow) the correct packet flows
   and error handling
   Implement the missing Tasks:
      - COM_STMT_PING  for connection keep alive
      - COM_STMT_BULK_EXECUTE for Bulk insert
      - COM_STMT_SEND_LONG_DATA for big data Insert/Update
      - COM_STMT_QUERY can use LOCAL INFILE syntax for File uploading. Implement this feature
@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented Feb 21, 2018

This looks great @SandorDobi thanks! Hopefully @Joannis can take a look soon

@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented Feb 21, 2018

Regarding macOS failing, you should be able to comment out that build (since Swift 4.1 is not yet supported there) just like is done here: https://github.com/vapor/vapor/blob/beta/circle.yml#L25

@Joannis

This comment has been minimized.

Copy link
Member

Joannis commented Feb 22, 2018

@SandorDobi where did you figure this out? There are close to no docs on this protocol as far as I could find.

@Joannis Joannis merged commit fd3b916 into vapor:beta Feb 22, 2018

1 check passed

ci/circleci: linux Your tests passed on CircleCI!
Details
@Joannis

This comment has been minimized.

Copy link
Member

Joannis commented Feb 22, 2018

This looks good, nothing that can be for the worse in any case. Just really curious where you got some of this info from 🥇

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