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

trigger reconnection when socket broken #38

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

wentianxiaokai
Copy link

This commit can fix the problem that old codes do not trigger reconnection when socket broken.The new codes can trigger reconnection when socket broken occur.

This commit can fix the problem that old codes do not trigger reconnection when socket broken.The new codes can trigger reconnection when socket broken occur.
Last solution is a wrong answer.The right answer is to add "erlang:monitor(process, RecvPid)," in mysql_recv.erl:73,after spawn_link()。
This commit can fix the problem that old codes do not trigger reconnection when socket broken.The new codes can trigger reconnection when socket broken occur.
@@ -75,6 +75,7 @@ start_link(Host, Port, LogFun, Parent) when is_list(Host), is_integer(Port) ->
spawn_link(fun () ->
init(Host, Port, LogFun, Parent)
end),
erlang:monitor(process, RecvPid),
Copy link

Choose a reason for hiding this comment

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

As I suggested in #37, I would move this call to mysql_conn:

    case mysql_recv:start_link(Host, Port, LogFun, self()) of
	{ok, RecvPid, Sock} ->
	    erlang:monitor(process, RecvPid), % <-- here
	    case mysql_init(Sock, RecvPid, User, Password, LogFun, FoundRows) of

Other than that suggestion, your change looks good to me.

@dhull
Copy link

dhull commented Mar 15, 2018

This pull request fixes an insidious bug in erlang-mysql-driver's closed-connection handling. I've written a longer description of the problem and why the fix works, which can perhaps be used as the commit message:

Fix handling of closed mysql socket.

mysql_recv sends mysql_conn a {mysql_recv, self(), closed, Error}
message and exits normally when it notices that the mysql connection
has been closed. Unfortunately, mysql_conn handles these "mysql_recv
closed" messages in several places but only itself exits if the
"mysql_recv closed" message is received in mysql_conn:loop (where it
is handled in the "Unknown" clause). If the "mysql_recv closed"
message is received in another location, the fact that the mysql_recv
process has exited is lost.

Furthermore, although mysql_conn is linked to mysql_recv, when
mysql_recv notices the closed connection it exits normally. That
means that mysql_conn ignores the exit signal from mysql_recv.

Finally, the mysql gen_server only removes the mysql socket from the
connection pool if the associated mysql_conn process itself
exits. (The mysql gen_server is monitoring the mysql_conn process
and receives a DOWN message when that happens.) That means that
although the mysql request that discovers that the connection has been
closed will return a "closed" error, the closed connection may be
returned to the connection pool to be used by the next request. At
this point, the next next request to use that connection will notice
that it is closed in the gen_tcp:send call and the code path in
mysql_conn will never exit, meaning that this connection will just
error forever.

This code change fixes the problem because it causes the mysql_conn
process to also receive a DOWN message when the mysql_recv process
exits and this message is received in only one place in the mysql_conn
process: in the "Unknown" clause in mysql_conn:loop. That means that
mysql_conn will always notice that the mysql_recv process has exited
and will always exit itself.

This code change also has the advantage over having mysql_recv just
exit with a non-normal exit status that the caller will always
receive a "closed" error instead of just having the call timeout,
which could happen if mysql_conn exited because of mysql_recv's
non-normal exit status before it handled the "mysql_recv closed"
message.

Base automatically changed from master to main January 24, 2021 21:44
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