Elixir loqui #3

jhowarth · 2016-11-29T01:59:33Z

I modeled this off of the websocket cowboy protocol.

Here's what the discord changes look like: https://github.com/hammerandchisel/discord/compare/feature/elixir_drpc?expand=1

jhowarth · 2016-11-29T01:59:58Z

@vishnevskiy @jhgg ready for a review, it passes the client benchmark test with 75k rps

vishnevskiy

Quick feedback, will have more

vishnevskiy · 2016-11-29T02:02:53Z

ex/loqui/lib/cowboy_protocol.ex

@@ -0,0 +1,215 @@
+defmodule Loqui.CowboyProtocol do
+  @opcode_hello 1


Can remove these from this file

vishnevskiy · 2016-11-29T02:03:40Z

ex/loqui/lib/cowboy_protocol.ex

+  @opcode_error 9
+
+  @supported_versions [1]
+  @supported_encodings MapSet.new(["erlpack"])


These should be passed in as options

vishnevskiy · 2016-11-29T02:05:19Z

ex/loqui/lib/parser.ex

+  end
+
+  def handle_data(<<@opcode_hello :: uint8, flags :: uint8, version :: uint8, psize :: uint32, payload :: binary-size(psize), rest :: binary>>) do
+    settings = parse_settings(payload)


If parsing fails an error should return so the consumer of the parser can close the socket. (aka not enough options)

vishnevskiy · 2016-11-29T02:09:08Z

ex/loqui/lib/cowboy_protocol.ex

+
+  def handler_init(%{transport: transport, req: req, handler: handler, handler_opts: handler_opts, env: env}=state) do
+    case handler.loqui_init(transport, req, handler_opts) do
+      {:ok, req2, ping_interval, handler_state} ->


You can call these req since its Elixir not Erlang.

vishnevskiy · 2016-11-29T02:10:21Z

ex/loqui/lib/cowboy_protocol.ex

+    end
+  end
+
+  def socket_data(state, data) do


Should this be private?

vishnevskiy · 2016-11-29T02:15:45Z

ex/loqui/lib/messages.ex

@@ -0,0 +1,44 @@
+defmodule Loqui.Messages do
+  # TODO: how to share these opcodes


Made an opcodes module that is just a macro then you can do use Loqui.Opcodes

vishnevskiy · 2016-11-29T02:16:05Z

ex/loqui/lib/messages.ex

+  @opcode_goaway 8
+  @opcode_error 9
+
+  # TODO: how to share these macros


Same as above but for types

jhgg · 2016-11-29T04:02:10Z

nit: I deliberately made encoding and compressions a list - as in the py/go implementations the ordering matters. The idea is that the client can specify the encodings it prefers first, before the encodings it supports. (Also, it's a bit overkill to use a MapSet for something that contains 1-2 things at most usually)

vishnevskiy · 2016-11-29T07:52:59Z

ex/loqui/lib/parser.ex

+    if setting == "" do
+      MapSet.new()
+    else
+      MapSet.new(String.split(setting, ","))


You should use lists, not sets for this because we need to preserve the order the clients send this data in.

vishnevskiy · 2016-11-29T07:53:37Z

ex/loqui/lib/types.ex

+defmodule Loqui.Types do
+  defmacro __using__(_) do
+    quote do
+      @opcode_hello 1


Why is this repeated in 2 places still?

vishnevskiy · 2016-11-29T07:54:12Z

ex/loqui/lib/parser.ex

+  end
+
+  defp parse_setting(setting) do
+    setting = String.trim(setting)


Do you need to trim? Can't you just match in the function.

vishnevskiy · 2016-11-29T07:56:26Z

ex/loqui/lib/cowboy_protocol.ex

+    handler.handle_request(request, handler_state)
+  end
+  defp handler_push(%{handler: handler, handler_state: handler_state}, request) do
+    handler.handle_push(request, handler_state)


We should be consistent either the prefix should be handle_ or loqui_

vishnevskiy · 2016-11-29T07:57:05Z

ex/loqui/lib/cowboy_protocol.ex

+  def handler_init(%{transport: transport, req: req, handler: handler, handler_opts: handler_opts, env: env}=state) do
+    case handler.loqui_init(transport, req, handler_opts) do
+      {:ok, req, handler_state, opts} ->
+        state = %{state | req: req, handler_state: handler_state} |> set_opts(opts)


Why not %{state | req: req, handler_state: handler_state} |> set_opts(opts) |> loqui_handshake

vishnevskiy · 2016-11-29T07:58:44Z

ex/loqui/lib/messages.ex

+defmodule Loqui.Messages do
+  use Loqui.{Opcodes, Types}
+
+  def make_hello_ack(flags, ping_interval, settings_payload) do


If this is a module for generating these values I feel like make_ prefix is redundant.

vishnevskiy · 2016-11-29T07:59:22Z

ex/loqui/lib/cowboy_protocol.ex

+
+  @default_ping_interval 5_000
+  @supported_versions [1]
+  @default_supported_encodings MapSet.new(["erlpack"])


Don't think we should have these defaults. I am going to ask Jake to remove it from the Python one. This should be always passed in for the config.

vishnevskiy · 2016-11-29T07:59:33Z

ex/loqui/lib/cowboy_protocol.ex

+  alias Loqui.{Parser, Messages}
+  require Logger
+
+  @default_ping_interval 5_000


This seems low.

vishnevskiy · 2016-11-29T08:00:14Z

ex/loqui/lib/cowboy_protocol.ex

+      !Enum.member?(@supported_versions, version) ->
+        goaway(state, :unsupported_version)
+        {:shutdown, :unsupported_version}
+      common_encodings == [] ->


Enum.empty?

vishnevskiy · 2016-11-29T08:01:28Z

ex/loqui/lib/cowboy_protocol.ex

+      true ->
+        flags = 0
+        encoding = List.first(common_encodings)
+        settings_payload = "#{encoding}|"


compressions not implemented yet?

vishnevskiy · 2016-11-29T08:03:55Z

ex/loqui/lib/cowboy_protocol.ex

+  end
+  defp handle_request({:request, _flags, seq, request}, state) do
+    request = decode(state, request)
+    {response, handler_state} = handler_request(state, request)


request and push should happen in a child process as they can do blocking operations. This library should handle having a pool of workers per connection.

I think the easiest route to take is that Elixir application that houses this code should just have a Supervisor as its main child that is either poolboy or gen stage? You can just feed it a job, monitor it and make sure it either responds to you or crashes. The same pool can be used across all clients unlike how we did it in Go and Python.

vishnevskiy · 2016-11-29T08:04:41Z

Does ping interval do nothing so far?

vishnevskiy

Looking good. Just need the pool of procs now.

vishnevskiy · 2016-11-30T04:57:13Z

ex/loqui/lib/cowboy_protocol.ex

+            ping_timeout_ref: nil
+
+  def upgrade(req, env, handler, handler_opts) do
+    {_, ref} = :lists.keyfind(:listener, 1, env)


Can't you just do env[:listener] in Elixir?

vishnevskiy · 2016-11-30T04:58:52Z

ex/loqui/lib/cowboy_protocol.ex

+      supported_compressions: supported_compressions}
+  end
+
+  def encode(%{encoding: "erlpack"}, msg), do: :erlang.term_to_binary(msg)


These shouldn't exist. The user of the library should encode and decode the data before talking to this library

vishnevskiy · 2016-11-30T05:20:29Z

ex/loqui/lib/messages.ex

@@ -0,0 +1,24 @@
+defmodule Loqui.Messages do


We didn't call these anything I think in the other code. But these are usually called "frames" in protocols I think?

vishnevskiy · 2016-11-30T23:33:30Z

For simplicity the pool should be started by this application (not a parent application), but expose pool configuration options on env.
Should just add typespecs for documentation.

Rest seems good.

vishnevskiy · 2016-12-01T00:46:57Z

Just typespecs then we golden!

jhowarth · 2016-12-01T01:26:20Z

Not sure if I did the typespecs right, but they look nice.

vishnevskiy · 2016-12-01T05:46:30Z

ex/loqui/lib/loqui/worker.ex

+  end
+
+  def handle_cast({:request, {m, f, a}, seq, from}, state) do
+    response = apply(m, f, a)


If this crashes does the caller know to send_error for that seq?

jhowarth added 6 commits November 28, 2016 15:03

add elixir

c62a4b7

Merge branch 'master' into ex

d00ffeb

support new version

b40fb3e

split message generation into its own file

40d8263

reject bad versions

30cb88a

code cleanup

bf4bc16

vishnevskiy reviewed Nov 29, 2016

View reviewed changes

jhowarth added 4 commits November 28, 2016 18:41

don't try to handle errors at the protocol level

acee56a

consolidate types and opcodes

a2459c5

elixir style

4f949ed

allow compressions and encodings to be specified

a248477

vishnevskiy suggested changes Nov 29, 2016

View reviewed changes

jhowarth added 6 commits November 29, 2016 11:47

code cleanup

da732f7

code cleanup from pr comments

23e177f

rename parser to protocol

a10573c

implement ping timeout

9ce1fbf

save compression

7a13c0e

return an error if parsing settings fails

a704708

vishnevskiy reviewed Nov 30, 2016

View reviewed changes

jhowarth added 6 commits November 30, 2016 13:28

poolboy

6452b03

remove handler state

8280c51

make the worker pool a parameter

3d5eb50

collect responses befoer flushing

fae1c4a

code cleanup

762c283

rename messages to frames

f6eea2d

jhowarth added 4 commits November 30, 2016 14:26

move encoding/decoding to drpc

6c252cb

set compressed flag

f1b65ec

code cleanup

722c472

code cleanup

31c6036

start poolboy as part of application

dbda6dd

add typespecs

bb4e7ff

vishnevskiy reviewed Dec 1, 2016

View reviewed changes

jhowarth added 5 commits December 1, 2016 16:18

handle worker errors

6f19cf2

code cleanup

f99d487

add spec

703c5ee

fix typespec

bfc20b4

flush properly

5aac4d2

jhowarth merged commit 3cbf8e6 into master Dec 2, 2016

jhowarth deleted the ex branch December 2, 2016 01:10

		@@ -0,0 +1,215 @@
		defmodule Loqui.CowboyProtocol do
		@opcode_hello 1

		@@ -0,0 +1,44 @@
		defmodule Loqui.Messages do
		# TODO: how to share these opcodes

Elixir loqui #3

Elixir loqui #3

Uh oh!

Conversation

jhowarth commented Nov 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhowarth commented Nov 29, 2016

Uh oh!

vishnevskiy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhgg commented Nov 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vishnevskiy commented Nov 29, 2016

Uh oh!

vishnevskiy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vishnevskiy commented Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vishnevskiy commented Dec 1, 2016

Uh oh!

jhowarth commented Dec 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jhowarth commented Nov 29, 2016 •

edited

Loading

vishnevskiy commented Nov 30, 2016 •

edited

Loading