Fix for OTP 21.3+. #253
Fix for OTP 21.3+. #253
Conversation
As of `[6168cf2]` in OTP 21.3, TLS/SSL sockets are active over top of the TCP sockets. For example, when a SSL socket is in passive mode, running an `:inet.getopts(tls_socket, [:active])` will show something like `[active,96]`, where-in `96` refers to the fourth packet received, starting at `100` (the default). However, Mariaex was originally written before this `active,N` implementation. As such, changing the TCP socket into `active,once` mode behaved "as expected". To my best understanding, Mariaex does this in order to "flush" the state buffers. Of course, with the `active,N` behaviour of SSL, once Mariaex puts the TCP socket into `active,once`, after the next message delivery, the SSL socket becomes passive. This seemed to create a situation where no further SSL data was made available. This change attempts to remove the transition into `active,once` and, instead, tries to keep the socket passive at all times. [6168cf2]: erlang/otp@6168cf2
| def tag(), do: :ssl | ||
|
|
||
| def fake_message(sock, buffer), do: {:ssl, sock, buffer} |
There was a problem hiding this comment.
My understanding here was that this was used to "flush" out anything that was currently in the socket buffer. Because that code path is gone, I do not believe there is any further use for this.
| @@ -240,7 +240,7 @@ defmodule Mariaex.Protocol do | |||
| {:error, error, _} -> | |||
| {:error, error} | |||
| {:ok, _, _, state} -> | |||
| activate(state, state.buffer) |> connected() | |||
| {:ok, %{state | buffer: state.buffer, state: :running}} | |||
There was a problem hiding this comment.
I believe this change has the same semantics as the original code.
| sock_mod.close(sock) | ||
| end | ||
| _ = sock_mod.recv_active(sock, 0, "") | ||
| sock_mod.close(sock) |
There was a problem hiding this comment.
This started to cause some troubles because there was no good way of knowing if disconnect is being called before or after the socket itself was actually closed. This created a difficult-to-debug situation where "left-over" data was stuck in the socket that the message decoder couldn't decode.
Then again, because the original code always closed the socket regardless of the actual response, it seems unnecessary to do the read, anyways.
| :ok -> sock_mod.recv_active(sock, 0, "") |> handle_recv_buffer(s) | ||
| {:disconnect, _, _} = dis -> dis | ||
| def checkout(%{sock: {sock_mod, sock}} = s) do | ||
| case sock_mod.recv(sock, 0, 0) do |
There was a problem hiding this comment.
This was somewhat inspired by the original receive_active code path. I'm not positive it maintains the same semantics, though, as I was never able to reproduce anything except a {:error, :timeout}. I considered just skipping this phase and returning {:ok, %{s | buffer:<<>>}}, but on the slim chance that there is data sitting in the socket, it seems worth clearing out.
| @@ -38,10 +38,14 @@ defmodule StartTest do | |||
| backoff_type: :stop] | |||
|
|
|||
| Process.flag :trap_exit, true | |||
| assert {:error, {%Mariaex.Error{message: "failed to upgraded socket: {:tls_alert, 'unknown ca'}"}, _}} = | |||
There was a problem hiding this comment.
The original tests didn't work properly for me. I suspect this was a result of the original SSL tests always being skipped, possibly having broken in the past. If I'm mistaken, this may come down to a MySQL error message inconsistency between releases.
3 similar comments
As of 6168cf2 in OTP 21.3, TLS/SSL sockets are active over top of
the TCP sockets. For example, when a SSL socket is in passive mode,
running an
:inet.getopts(tls_socket, [:active])will show somethinglike
[active,96], where-in96refers to the fourth packet received,starting at
100(the default).However, Mariaex was originally written before this
active,Nimplementation. As such, changing the TCP socket into
active,oncemode behaved "as expected". To my best understanding, Mariaex does this
in order to "flush" the state buffers. Of course, with the
active,Nbehaviour of SSL, once Mariaex puts the TCP socket into
active,once,after the next message delivery, the SSL socket becomes passive. This
seemed to create a situation where no further SSL data was made
available.
This change attempts to remove the transition into
active,onceand,instead, tries to keep the socket passive at all times to fix #249.