Remove handle_request from Raxx.Server behaviour

nietaki commented 5 years ago

Problem

handle_request is a convenience interface that can (and is, in practice) be implemented on top of handle_head, handle_data and handle_tail.

Allowing users to implement all 4 callbacks is confusing - they are basically exclusive and it is not obvious which callback will actually be called in the end. Similarly, if you use Raxx.Server and mix and match callbacks you choose to override, you'll most likely break your server - all 3 "basic" ones, implemented here need to be left intact or overridden in order for the server to behave predictably.

Another good example why it doesn't belong in the interface is inside Raxx.Router - the Router needs to ignore handle_request, because it's actually irrelevant in the inner workings of Raxx.

Moreover, if you don't use Raxx.Server, but rely on the behaviour itself, you're forced to implement handle_request callback, even though it's never actually going to be called. This, in my opinion, conflicts with the idea of Raxx being the interface to build on top of.

Proposed solution

I'll think Raxx.Server should have just handle_head, handle_data and handle_tail and handle_info as callbacks, whereas handle_request could be the only callback of something akin to Raxx.SimpleServer.

The onboarding experience for users who want to work on the request/response paradigm could be just as simple: use Raxx.SimpleServer or maybe use Raxx.Server, :simple would force the user to implement the Raxx.SimpleServer behaviour while translating it to Raxx.Server behaviour using not overridable functions in __using__ - it would be impossible to break the "translation" or not implement handle_request.

This way a user wouldn't really need to use Raxx.Server, just @behaviour Raxx.Server would guide them through which callbacks they're missing. We could keep use Raxx.Server and have it be as simple as

  defmacro __using__(options) do
    quote do
      @behaviour unquote(__MODULE__)

      import Raxx
      alias Raxx.{Request, Response}
    end
  end

Notes

I know this is a breaking change and would require a breaking release. I still think it's worth it, for much cleaner semantics in the future.
The issue might be phrased a little chaotically, but I just needed to get it off mind. I'll be happy to clarify or try to argue the point better.

CrowdHailer commented 5 years ago

This change should definitely happen. I can't think of any good arguments for keeping it as it currently is.

We need to consider the fact that handle_info is part of both the Server and SimpleServer interface. I don't know if something can be part of two behaviours or not, so might be awkward if someone wants to implement handle_request and handle_info and use @impl

The other thing of note, maybe you already spotted this. I don't think we can call the full interface streaming, because using just handle_request and handle_info is how to make a server that streams a response, It's how we work with server sent events.

Finally is this a prerequisite of working out middleware? Ideally I would like to add the middleware experiment before tackling this issue

nietaki commented 5 years ago

I totally missed the fact that handle_info is relevant to the handle_request approach, thanks for pointing that out.

I'll think how to best reconcile handle_info being in both interfaces. My best idea right now is handle_request being the only callback in SimpleServer behaviour, and having use SimpleServer leave handle_info callback of Server unimplemented or implemented but overridable.

This is not a prerequisite for middleware, I just needed to get it out in the open, put it on the roadmap and simplify my mental model in advance ;)

Also, good point on

don't think we can call the full interface streaming

I don't have a naming scheme in mind apart from Server/normal/full and SimpleServer/simplified. Let's iron it out further down the line.

Note to my future self: when the interface split happens (or even before that) it would be good to emphatically document the possible values of Raxx.Request body field for handle_head (only true or false) and handle_request (only a binary or false) callbacks. I was almost sure that's how it worked, but I wasn't absolutely certain.

CrowdHailer / raxx