Adds support for allowing PubSub subscription and events handling #189

guidotripaldi · 2019-08-06T08:18:07Z

PubSub subscription requires the PID of the main process, but in the
Commander the events callbacks are spawned asynchronously and a
temporary PID is assigned to them, different from the main one.
For this reason we need a synchronous callback to be called in the
Commander where to put the call to the PubSub subscription.
This new synchronous callback onload_init, is called just before
calling the onload callback.

The GenServer handle_info PubSub events are handled calling
the handle_info_message/2 callbacks, that is expected to be present
in the Commander for any different combination of topic/message
subscribed.

The handle_info_message/2 callback in the Commander expect a
message parameter, identical to the standard handle_info/2 one,
and a socket parameter, instead of the GenServer state parameter.
It haven't to return anything.

The original state parameter will be returned by the internal handling
of the GenServer/PubSub event.

PubSub subscription requires the PID of the main process, but in the Commander the events callbacks are spawned asynchronously and a temporary PID is assigned to them, different from the main one. For this reason we need a synchronous callback to be called in the Commander where to put the call to the PubSub subscription. This new synchronous callback `onload_init`, is called just before calling the `onload` callback. The GenServer `handle_info` PubSub events are handled calling the `handle_info_message/2` callbacks, that is expected to be present in the Commander for any different combination of topic/message subscribed. The `handle_info_message/2` callback in the Commander expect a message parameter, identical to the standard `handle_info/2` one, and a socket parameter, instead of the GenServer `state` parameter. It haven't to return anything. The original `state` parameter will be returned by the internal handling of the GenServer/PubSub event.

OvermindDL1 · 2019-08-06T15:42:47Z

Isn't this pattern already covered via Drab's other broadcast methods? Do you have a specific example that this enabled that cannot be done with currently existing Drab styles?

guidotripaldi · 2019-08-07T11:22:17Z

Yes it is possible to use Drab broadcast/subscribe functions for triggering updates from other parts of an application, but with some important issues compared to having a native support for PubSub events.

For example consider this common situation: you have a Backend module for managing database CRUD operations and other administrative tasks as well, a Frontend module for rendering the web pages, an API server for interfacing with iOS/Android apps or other webservices. Sources of database changes can be both user interactions with web pages or smartphone app, or some administrative background tasks. You want that the web pages as well the iOS/Android/services will be updated/notified every time a change occours on the database.

Without PubSub, we have to centralise in the Backend all the calls to every function of all part of your application that needs signaling when data updates, for example:

defmodule MyApp.Backend.Users do
  ...
  # CRUD operations
  def update_user(user, attrs) do
     ...
     |> Repo.update()
     |> notify_subscribers([:user, :updated])
  end
  ...
  # Notifications stuff
  defp notify_subscribers(result, event) do
    event
    |> notify_admin(result)
    |> notify_api(result)
    |> notify_drab(result)
  end

  defp notify_drab(event, result) do
    ...
    # Set up all the assigns you would need to update in the various templates
    # also if those particular template isn't active 
    # at this time.
    # Every time a template is added or its assigns modified, you have 
    # to reflect here the changes.

    user = ...
    user_stats = ...
    users = ...
    users_stats = ...

    # SignOn page
	Drab.Live.broadcast_poke(Drab.Core.same_topic("users-changed"), MyApp.Frontend.SignOnView, "index.html", user: user, using_assigns: [<put here all other assigns used in this template that haven't to be changed>])

    # Account page
	Drab.Live.broadcast_poke(Drab.Core.same_topic("user-changed"), MyApp.Frontend.AccountView, "edit.html", user: user, using_assigns: [<put here all other assigns used in this template that haven't to be changed>])
	Drab.Live.broadcast_poke(Drab.Core.same_topic("user-changed"), MyApp.Frontend.AccountView, "show.html", user: user, using_assigns: [<put here all other assigns used in this template that haven't to be changed>])

    # Admin
	Drab.Live.broadcast_poke(Drab.Core.same_topic("users-changed"), MyApp.Frontend.AdminView, "index.html", users: users, using_assigns: [<put here all other assigns used in this template that haven't to be changed>])
	Drab.Live.broadcast_poke(Drab.Core.same_topic("user-changed"), MyApp.Frontend.AdminView, "edit.html", user: user, using_assigns: [<put here all other assigns used in this template that haven't to be changed>])

    # Stats page
	Drab.Live.broadcast_poke(Drab.Core.same_topic("users-changed"), MyApp.Frontend.SignOnView, "index.html", users: users, users_stats: users_stats, using_assigns: [<put here all other assigns used in this template that haven't to be changed>])
	Drab.Live.broadcast_poke(Drab.Core.same_topic("user-changed"), MyApp.Frontend.SignOnView, "show.html", user: user, user_stats: user_stats, using_assigns: [<put here all other assigns used in this template that haven't to be changed>])

    ... # and so on for every peculiar combination of event/page/assignments

  end
  ... # repeat as above for every other Module which need to trigger updates
end

As we can see, this way we have three big issues: a broken separation of concerns, increasing inefficiency and poor manutenibility:

separation of concerns

The Backend (that in an umbrella project could be a different app from the web Frontend) have to be aware that Frontend pages (or any part of the app) have to be updated and how. This brokes one of the main principles of any good structured application.

inefficiency

Since we cannot know in the Backend (and we haven't to!) which are the current active pages, and which are the specific assigns that actually need updates, we always have to set up all of them. In the above example, we must calculate the user and users stats also if nobody is watching at the stats pages now, just to be able to serve those pages who need to be updated when changes to users table occur.

poor manutenibility

Since we have to esplicitily set up a Drab.Live.broadcast_poke/4 call (or other kinds of Drab broadcast call) for every peculiar combination of event/page/assignments, and this for every database table/module or other part of the application could signal a change, the code base becomes easily and quickly unnecessarily big and poorly maintainable.

instead, using PubSub, we guarantee separation of concerns, efficiency and easy manutenibility as we can decentralise all the functions and their details on how to update clients to the appropriate contexts. For example:

defmodule MyApp.Backend.Users do
  ...

  # CRUD operations
  def update_user(user, attrs) do
     ...
     |> Repo.update()
     |> notify_subscribers([:user, :updated])
  end
  ...
  # PubSub notification stuff
  def subscribe() do
  	Phoenix.PubSub.subscribe(MyApp.PubSub, @topic)
  end

  def subscribe(id) do
  	Phoenix.PubSub.subscribe(MyApp.PubSub, @topic<>"-#{id}")
  end

  def notify_subscribers({:ok, user}, event)
    Phoenix.PubSub.broadcast(MyApp.PubSub, @topic, {"@topic", event, user})
    Phoenix.PubSub.broadcast(MyApp.PubSub, @topic <> "-#{user.id}", {@topic, event, user})
  end
... # repeat as above for every other Module which need to trigger updates
end



defmodule MyApp.Frontend.SignupCommander do
  ...
  defhandler create_user_button_clicked(socket, sender) do
    attrs = ...
    # will create a user and trigger an update events for all Backend.Users subscribers
    MyApp.Backend.Users.create_user(attrs)
  end
end

defmodule MyApp.Frontend.StatsCommander do
  ...
  # setup
  onload_init: do_oninit

  def do_oninit(socket) do
    MyApp.Backend.Users.subscribe()
  end
  ...
  # handle pubsub stuff
  def handle_info_message({@topic, [:user, updated], _}, socket) do
    calc_users_stat()
    |> update_users_stats_page(socket)
  end
  ...
  # helpers
  defp calc_users_stats() do
    ...
  end

  def update_users_stats_page(stats, socket) do
  	broadcast_poke(socket, users_stats: stats)
  end
end

defmodule MyApp.Frontend.AccountCommander do
  ...
  # setup
  onload_init: do_oninit

  def do_oninit(socket) do
    current_user = ...
    MyApp.Backend.Users.subscribe(current_user.id)
  end
  ...
  # handle pubsub stuff
  def handle_info_message({@topic, [:user, updated], user}, socket) do
    user
    |> calc_user_stat()
    |> update_user_stats_page(socket)
  end
  ...
  # helpers
  defp calc_user_stats() do
    ...
  end

  def update_user_stats_page(stats, socket) do
    broadcast_poke(socket, user_stats: stats)
  end
end

Thus, as we all surely love to apply the principles of separation of concerns and an idiomatic approach in our applications, I think we need supporting PubSub in Drab. Otherwise Drab could be not the best choice for a more complex application, like the above exemplified.

The Drab subscribe/broadcast native mechanism is still valid and the preferred way for managing all of the other situations that doesn't need a separation of concerns between different parts of the application.

OvermindDL1 · 2019-08-07T15:07:03Z

Without PubSub, we have to centralise in the Backend all the calls to every function of all part of your application that needs signaling when data updates, for example:

I have not and I have a similar setup in a couple of my projects; first thing is for broadcasts I make a dedicated genserver under my supervisor that listens for pubsub requests, and receiving them it transforms the data into the format useful to put on a web page and then it broadcasts 'that' to the pages. My backend's do not know that drab even exists. and data flows around in its native format, and instead of being broadcast out to N processes that will each then mutate the data then rebroadcast it out to the page (including doing this in the 'main' socket, which you absolutely do not want to do anything that could potentially take time in), which is more efficient by far.

separation of concerns

The Backend (that in an umbrella project could be a different app from the web Frontend) have to be aware that Frontend pages (or any part of the app) have to be updated and how. This brokes one of the main principles of any good structured application.

Your backend should not be talking to drab, it should be talking to pubsub, and drab listens to pubsub, say via a genserver.

inefficiency

Since we cannot know in the Backend (and we haven't to!) which are the current active pages, and which are the specific assigns that actually need updates, we always have to set up all of them. In the above example, we must calculate the user and users stats also if nobody is watching at the stats pages now, just to be able to serve those pages who need to be updated when changes to users table occur.

I don't see why you would need to, just broadcast to the pubsub as normal and if nothings listening then no issue. I don't see why you need to calculate that each time as a genserver could listen to update messages and update the stats as necessary, broadcasting them out to the pages if any (which is far better than doing that N times, once for each open socket).

poor manutenibility

Since we have to esplicitily set up a Drab.Live.broadcast_poke/4 call (or other kinds of Drab broadcast call) for every peculiar combination of event/page/assignments, and this for every database table/module or other part of the application could signal a change, the code base becomes easily and quickly unnecessarily big and poorly maintainable.

This has to be done regardless, whether inside an existing socket or not, however by broadcasting it to a pubsub that is listened to by each main socket process, that means you have to work on that data N times instead of just once.

instead, using PubSub, we guarantee separation of concerns, efficiency and easy manutenibility as we can decentralise all the functions and their details on how to update clients to the appropriate contexts. For example:

Using a genserver process to listen for pubsub events then work on the data there and broadcast to the pages means you aren't doing the same work N times, so it is significantly more efficient, you have a proper separation of concerns (Drab should not need to care about where the data comes from, it only manages the webpage), maintainability is increased due to the transformation process, and this is the BEAM way, transforming messages. :-)

  # handle pubsub stuff
  def handle_info_message({@topic, [:user, updated], _}, socket) do
    calc_users_stat()
    |> update_users_stats_page(socket)
  end

Yeah this part is horribly inefficient, that's doing work in the socket process. The main Drab socket process is like the main Phoenix.Channel main process, you must never ever perform work in it as it effectively freezes the client communication when that is happening, in addition you are performing the same work N times instead of just once. This is precisely the same reason you aren't allowed to do anything in a Phoenix.Channel main process either and instead have to use topic processes.

The Drab subscribe/broadcast native mechanism is still valid and the preferred way for managing all of the other situations that doesn't need a separation of concerns between different parts of the application.

This isn't a separation of concerns though, the above example is putting message transformations 'into' the main socket process in such a way that will prevent client messages and actions while they are processing and, more importantly in this case, doing the work N times instead of only once.

I'm entirely open to having this merged if there is a valid reason, but I'm not seeing one yet, and the above example shows that people would use it for the wrong reasons and hamper efficiency and maintainability.

I haven't had time to review this yet to check how it is implemented yet honestly, didn't have time yesterday, though I do now, so one moment. :-)

OvermindDL1

Added notes.

Although I still don't think this is best as it stands. Working on messages should be done via a transformation process, it really really shouldn't be duplicated across N connections... If you want to know whether something is listening for a message that's why Drab.Presence exists as well, so you know whether it's worth doing the work to calculate something for broadcasting (though if its super cheap to calculate just broadcast it anyway, if nothings listening then it's not any real cost).

OvermindDL1 · 2019-08-07T15:37:17Z

lib/drab.ex

@@ -213,6 +213,15 @@ defmodule Drab do
    {:noreply, state}
  end

+  # pass any other handle_info to the Commander `handle_info_message/2` callbacks,
+  # which expect `message` and `socket` params (no `state` reply).
+  def handle_info(message, state) do


I'm not sure this should be in Drab, it should probably be in Drab.Channel, honestly Drab shouldn't be a genserver at all, everything it does and all the information it holds really should be in Drab.Channel, but that's a refactor for another day. ^.^;

For note, Drab.Channel already handles phoenix messages, although it passes them to the client for bouncing back 'through' drab, that should probably be restricted to a specific set of 'types' of topics for such javascript bouncing, so normal phoenix events can be handled differently. Also, Drab.Commander has subscribe and unsubscribe calls for registering to phoenix pubsub topics through the endpoint via Drab.Channel. I wonder what messages it handles through here actually, I.E. I wonder what the purpose of it is to send only to the JS side, hmm... This needs to be researched. Maybe Drab already has this functionality but it could just be 'fleshed out' a little bit...

Maybe Drab already has this functionality but it could just be 'fleshed out' a little bit...

maybe, for now I haven't found a better way to support PubSub natively in a Commander

OvermindDL1 · 2019-08-07T15:37:53Z

lib/drab.ex

+  def handle_info(message, state) do
+    state.socket
+    |> get_commander()
+    |> apply(:handle_info_message, [message, state.socket])


This is synchronous! Definitely shouldn't be! Drab has async calls in this module, should follow the same pattern as them.

Also, why is this name of handle_info_message hardcoded? Although I like hardcoded names, that's not Drab's way.

it is hardcoded as well as GenServer name handle_info/2' callback is. I've choose not to call it just "handle_info" because though its role is similar, it is not identical (because it receive a socketinstead of astate, and doesn't return a state`. Naming is difficult, so certainly we can choose another better name (actually I don't like too much 'handle_info_message' but after thinking a lot I couldn't find a better one). It have to be hardcoded because there is no other way, as far as I know, to have many of this kind of handlers (each one with it own pattern matching on message), in a Commander.

OvermindDL1 · 2019-08-07T15:40:52Z

lib/drab.ex

+
+  @spec handle_callback_sync(atom, atom) :: any
+  defp handle_callback_sync(commander, callback) do
+    if callback, do: apply(commander, callback, [])


This really shouldn't exist, like really really shouldn't exist.

If someone wants the pid to drab they can just call the Drab.pid(socket) as normal in the onload callback (or any other callback). The presence of this function makes it easy to accidentally perform complex operations and implies they can only get the pid via this function instead of just using Drab.pid(socket) as normal.

This is necessary to let to subscribe to PubSub in the Commander. This because it is deprecated to pass a PID to the PubSub.subscribe function, otherwise we could just use onload event and pass the Drab.pid(socket) to PubSub. This is the reason behind the choice to have a new sync handler. I don't really like it either, but afaik there is no other ways to implement a pubsub subscription from inside a Commander.

OvermindDL1 · 2019-08-07T15:41:09Z

lib/drab/commander.ex

@@ -132,13 +132,22 @@ defmodule Drab.Commander do
      defmodule DrabExample.PageCommander do
        use Drab.Commander

+        onload_init: do_init


Should not exist, it is sync.

OvermindDL1 · 2019-08-07T15:41:18Z

lib/drab/commander.ex

        onload :page_loaded
        onconnect :connected
        ondisconnect :disconnected

        before_handler :check_status
        after_handler  :clean_up, only: [:perform_long_process]

+        def do_init do


Ditto, no sync calls.

OvermindDL1 · 2019-08-07T15:44:09Z

lib/drab/commander.ex

@@ -191,6 +208,11 @@ defmodule Drab.Commander do
  Runs after the event handler. Gets return value of the event handler function as a third argument.
  Can be filtered by `:only` or `:except` options, analogically to `before_handler`

+
+  ### `handle_info_message`


This seems to focus a lot on 'pubsub' when it is nothing of the sort, only thing it does is pass in otherwise unhandled messages, I.E it's an info message handler, no need to focus on pubsub stuff and just say that when an unknown message is sent to the drab pid then it is received here. Phoenix.PubSub already clarifies how Phoenix.PubSub works.

Yes it is focused on PubSub because this is the main and most useful reason to add this stuff to Drab, but of course we change the documentation.

OvermindDL1 · 2019-08-07T15:45:31Z

lib/drab/commander.ex

@@ -240,6 +262,92 @@ defmodule Drab.Commander do
  Drab injects function `render_to_string/2` into your Commander. It is a shorthand for
  `Phoenix.View.render_to_string/3` - Drab automatically chooses the current View.

+  ## PubSub support


Technically Drab already has PubSub support as of https://github.com/grych/drab/releases/tag/v0.8.3 although not of the form you are expecting, rather it passes messages to the javascript to then be handled there. Messages to the Drab process should already be web-ready regardless, unknown pubsub messages should never reach drab regardless.

It seems that Drab hasn't any kind of PubSub support. It just use PubSub for its internal uses, that it is a different things to let the developer uses Phoenix.PubSub directly inside a live page if needed, as for example LiveView does and suggest.

OvermindDL1 · 2019-08-07T15:46:10Z

lib/drab/commander/config.ex

@@ -4,6 +4,7 @@ defmodule Drab.Commander.Config do
  defstruct commander: nil,
            controller: nil,
            view: nil,
+            onload_init: nil,


This is a synchronous call, should not exist.

OvermindDL1 · 2019-08-07T15:48:49Z

test/support/router.ex

@@ -49,6 +49,8 @@ defmodule DrabTestApp.Router do
    get("/tests/live/broadcasting", LiveController, :broadcasting, as: :broadcasting)

    get("/tests/element", ElementController, :index, as: :element)
+
+    get("/tests/pubsub", PubsubController, :index, as: :pubsub)


Drab already has pubsub functionality, although it's tested in the LiveController test. It really should be pulled out into it's own test, and Drab.Channel's message receiver should be changed so if the topic starts with drab: then it should do as it does now, else it should call a callback in the appropriate commander (which could potentially be shared commanders).

I don't understand this comment. Here we are just testing the new added functionalities with a PubSub lifecycle case, thus the name 'pubsub' for this test.

Oh I was just saying that the little bit of pubsub use that drab uses is testing in the /tests/live/broadcasting route, when they should probably be pulled out into their own. ^.^

OvermindDL1 · 2019-08-07T15:50:23Z

lib/drab/commander.ex

+          @topic inspect(__MODULE__)
+
+          def subscribe() do
+            Phoenix.PubSub.subscribe(MyApp.PubSub, @topic)


This isn't taking a PID, that's very weird...

It is not possible to pass the pid to a PubSub.subscription function. Otherwise we could just call Phoenix.PubSub.subscribe(MyApp.PubSub, Drab.pid(socket), @topic) in the onload handler, without any need of the added sync call.

This because, although documented, Phoenix.PubSub.subscribe/3 is deprecated. If used it works for now, but warnings with: "Passing a Pid to Phoenix.PubSub.subscribe is deprecated. Only the calling process may subscribe to topics" so we cannot anymore use it.

This is the exact reason because I've elaborate a solution through the onload_init stuff.

It is not possible to pass the pid to a PubSub.subscription function.

https://hexdocs.pm/phoenix_pubsub/Phoenix.PubSub.html#subscribe/3
Why not? It's not marked as deprecated in the docs. Nor does it have a @deprecated annotation.

Besides, Drab already has a method to subscribe to pubsub topics via it's own subscribe function, it just needs to detect drab specific things and 'other' things then route the others back to the commands instead.

https://github.com/phoenixframework/phoenix_pubsub/blob/v1.1.2/lib/phoenix/pubsub.ex#L154

guidotripaldi · 2019-08-07T18:33:31Z

Without PubSub, we have to centralise in the Backend all the calls to every function of all part of your application that needs signaling when data updates, for example:

I have not and I have a similar setup in a couple of my projects; first thing is for broadcasts I make a dedicated genserver under my supervisor that listens for pubsub requests, and receiving them it transforms the data into the format useful to put on a web page and then it broadcasts 'that' to the pages. My backend's do not know that drab even exists. and data flows around in its native format, and instead of being broadcast out to N processes that will each then mutate the data then rebroadcast it out to the page (including doing this in the 'main' socket, which you absolutely do not want to do anything that could potentially take time in), which is more efficient by far.

yes, I've deliberately omitted to talk about this kind of solution because of two reason: to avoid overloading the already long previous post, and because I don't think this is a good (general) solution.

In my opinion, having a new layer of complexity just to support PubSub instead of having the possibility of a direct use from inside a Commander, it is a bad thing. Of course in certain cases could be the right solution, but non for the general case. I say this, imho, for some reasons:

not having the chance to use natively PubSub in a Phoenix application is not idiomatic;
libraries have to decrease complexity, not to increase it. If we want to attract more developers, Drab has to offer simpler way to do commons things;
LiveView, though is less powerful than Drab for now, is a good reference in terms of usability. It allows and incite to use PubSub directly in their "Commander" without the need of any supplemental GenServer stuff. And having been developed by the creator of Phoenix, I think we cannot say that this approach isn't good.

OvermindDL1 requested changes Aug 7, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for allowing PubSub subscription and events handling #189

Adds support for allowing PubSub subscription and events handling #189

guidotripaldi commented Aug 6, 2019

OvermindDL1 commented Aug 6, 2019

guidotripaldi commented Aug 7, 2019 •

edited

OvermindDL1 commented Aug 7, 2019

OvermindDL1 left a comment

OvermindDL1 Aug 7, 2019

guidotripaldi Aug 7, 2019

OvermindDL1 Aug 7, 2019

OvermindDL1 Aug 7, 2019

guidotripaldi Aug 7, 2019

OvermindDL1 Aug 7, 2019

guidotripaldi Aug 7, 2019 •

edited

OvermindDL1 Aug 7, 2019

OvermindDL1 Aug 7, 2019

OvermindDL1 Aug 7, 2019

guidotripaldi Aug 7, 2019

OvermindDL1 Aug 7, 2019

guidotripaldi Aug 7, 2019

OvermindDL1 Aug 7, 2019

OvermindDL1 Aug 7, 2019

guidotripaldi Aug 7, 2019 •

edited

OvermindDL1 Aug 7, 2019

OvermindDL1 Aug 7, 2019

guidotripaldi Aug 7, 2019

OvermindDL1 Aug 7, 2019 •

edited

guidotripaldi Aug 8, 2019

guidotripaldi commented Aug 7, 2019

Adds support for allowing PubSub subscription and events handling #189

Are you sure you want to change the base?

Adds support for allowing PubSub subscription and events handling #189

Conversation

guidotripaldi commented Aug 6, 2019

OvermindDL1 commented Aug 6, 2019

guidotripaldi commented Aug 7, 2019 • edited

separation of concerns

inefficiency

poor manutenibility

OvermindDL1 commented Aug 7, 2019

OvermindDL1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guidotripaldi Aug 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guidotripaldi Aug 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OvermindDL1 Aug 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guidotripaldi commented Aug 7, 2019

guidotripaldi commented Aug 7, 2019 •

edited

guidotripaldi Aug 7, 2019 •

edited

guidotripaldi Aug 7, 2019 •

edited

OvermindDL1 Aug 7, 2019 •

edited