Auth required #36
Auth required #36
Conversation
…command to test connection to the server.
…ication is running
|
This probably fails the Travis CI checks because the test I wrote needs to a second server running that requires authentication. |
|
@tallguy-hackett you could get travis to pass by adding a |
|
Looks like you and @film42 have both been thinking about this problem: https://github.com/mmmries/gnat/compare/gt/parse_info_and_ok?expand=1 I like that both of you pushed the parsing into |
| :ok = Gnat.ping(gnat) | ||
| ``` | ||
| """ | ||
| def ping(pid) do |
There was a problem hiding this comment.
Exposing this as usable behavior seems odd to me. What happens for instance if two different clients both send call Gnat.ping close to the same time? The state.pinger would end up holding the address of the second client and the first one would timeout waiting for a PONG.
I think it probably makes sense for the Gnat actor to send a PING every so often and expect a response, but I don't understand the purpose of making it a callable behavior. Maybe I'm missing something?
There was a problem hiding this comment.
This is actually pretty common on nats clients, but the method is called flush: https://github.com/nats-io/go-nats/blob/master/nats.go#L2464-L2468
| } | ||
| {:ok, pid} = Gnat.start_link(connection_settings) | ||
| assert Process.alive?(pid) | ||
| :ok = Gnat.ping(pid) |
There was a problem hiding this comment.
oh, is this the reason you wanted expose ping as a public function? I guess it's a pretty good way to verify that the connection was successful... It's certainly simpler than doing a sub + pub + receive
test/test_helper.exs
Outdated
| :gen_tcp.close(socket) | ||
| {:error, reason} -> | ||
| Mix.raise "Cannot connect to gnatsd" <> | ||
| " (http://localhost:4223):" <> |
There was a problem hiding this comment.
I like the helpful error message here, but it seems like a pain to force all developers to run two gnatsd servers while developing. Maybe this is a good time to tag our tests and default the integration tests to be skipped
| receive do | ||
| {:tcp, ^tcp, operation} -> | ||
| "INFO" = operation |> String.split |> List.first |> String.upcase | ||
| :gen_tcp.send(tcp, "CONNECT {\"verbose\": false}\r\n") | ||
| {_, [{:info, options}]} = Parser.parse(Parser.new, operation) |
There was a problem hiding this comment.
As far as I know, there's no guarantee that the entire INFO message will fit in this first TCP packet, so I think moving the message parsing into Parser is a good start, but I think it might be better to let the genserver receive packets until the parser finds the INFO message. And then maybe we wait for a message from the genserver in init to ensure the handshake completed successfully?
lib/gnat/parser.ex
Outdated
| @@ -1,5 +1,6 @@ | |||
| defmodule Gnat.Parser do | |||
| require Logger | |||
| require Poison | |||
There was a problem hiding this comment.
I don't think we need to require poison to use the parser.
…king before a multi server test is run to verify another server is up.
|
I added some functionality for tagging tests as well as dynamically checking for a second server when including the |
| after 1000 -> | ||
| {:error, "timed out waiting for info"} | ||
| end | ||
| end | ||
|
|
||
| defp connect(tcp, %{auth_required: true}=_options, %{username: username, password: password}=_connection_settings) do |
There was a problem hiding this comment.
I don't think we need to assign unused variables _connection_settings and _options here. The pattern matching should be just fine and if someone wanted context about what is being matched, they can look and the other connect function
This fixes #27
I added functionality to pass connection information into the parser to generate the
CONNECTcommand. This may not be the best way to do it so I'm very open to suggestions. I also added aGnat.pingfunction that sends aPINGcommand to the server to help verify an authenticated connection without a need for a subscription request.There's also no error handling if the client tries to use a connection without authenticating.