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

Handle error when "query" is of shape %{opts: opts}. Should fix #250. #255

Closed
wants to merge 1 commit into from
Closed

Conversation

derekkraan
Copy link

@derekkraan derekkraan commented Jun 26, 2019

Here is my commentary from #250, to give some context:

I have looked at the code to try to understand what is going on here. handshake_recv/2 is called like so on line 103: handshake_recv(s, %{opts: opts})
Then on line 171 this %{opts: opts} gets sent to handle_handshake/3 as second parameter.
There are three function heads for handle_handshake/3. As far as I can see it's supposed to come out to the one first one, defined on line 202, but it ends up in the third one, defined on line 231.

This is where I lose the plot a little bit, but the case statement on line 1245 is not set up to handle %{opts: opts}, so it crashes.

I can't tell if it's supposed to get to handle_handshake/3 on line 231. Possibly the answer is yes (?). If it is supposed to be able to get there, then there needs to be a case clause added to abort_statement to handle %{opts: opts} as query.

Please advise if there is any additional cleanup that needs to happen. My gut says that there isn't, but I'm also not very familiar with the mariaex internals.

@coveralls
Copy link

Coverage Status

Coverage increased (+2.5%) to 86.022% when pulling 09a281d on derekkraan:master into 5f4aa50 on xerions:master.

@coveralls
Copy link

coveralls commented Jun 26, 2019

Coverage Status

Coverage increased (+3.2%) to 86.738% when pulling 578fcd4 on derekkraan:master into 5f4aa50 on xerions:master.

@derekkraan
Copy link
Author

I have tried running the tests a couple of times but there is always one test that fails in one of the (26) runs. Perhaps you could rerun just that one test for me @xerions?

@derekkraan
Copy link
Author

@fishcakez Could you please take a look at this PR? I believe the impact is rather large; it can bring down an entire application if the database starts to refuse new connections (for whatever reason).

@derekkraan derekkraan closed this by deleting the head repository Feb 3, 2023
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