Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite API for hooking into requests and responses. #136

Open
OrangeTux opened this issue Oct 15, 2020 · 1 comment
Open

Rewrite API for hooking into requests and responses. #136

OrangeTux opened this issue Oct 15, 2020 · 1 comment
Labels
enhancement New feature or request

Comments

@OrangeTux
Copy link
Collaborator

OrangeTux commented Oct 15, 2020

Currently this package provides 2 ways of routing calls:

  • @on() - Main handler for a call. It's response is send back as a other end of the connection.
  • @after() - Optional handler that is executed after the @on() is executed. It receives same arguments as @on() but its response is discarded.

The current options for routing are limited and have problems.

First the problems:

  1. @after() handlers are not executed if the in corresponding @on() handler an exceptions is raised. This behavior is not clear from API.
  2. @after() handlers require a corresponding @on() handler, otherwise they never will be executed. Again this is an unclear, implicit relation between the @on() and the @after() handler.
  3. Exceptions that are raised in @after() can't be catched.
  4. @after() handlers are 'fired and forgotten' because the coroutines are started using asyncio.ensure_future. This could lead to long running tasks in the background without that a user knows that.

Now the limitations:

  1. There is no way to access the request before it's passed to the @on() handler. Nor it's possible to access the response before it's put on the wire.

Here some use cases that aren't possible because of the current problems:

A) Logging requests and responses to a different place than stdout. For example at the Mobility House we want to store certain requests in a database. That isn't possible without relying on internals of ChargePoint.
B)Tracking the execution time of a @on() handler.
C) Catch exceptions raised in an @on() handler.

I think the @on() is working fine. But we need to change the API to allow users to hook into requests and responses before and after the @on() handler.

@OrangeTux OrangeTux changed the title New Rewrite API for hooking into requests and responses. Oct 15, 2020
@OrangeTux OrangeTux added the enhancement New feature or request label Oct 15, 2020
@OrangeTux
Copy link
Collaborator Author

One way of modifying requests and or responses is to wrap to apply a decorator to a handler.

Consider the following decorator that measures the execution time of handlers:

def time_it(f):
   def inner(*arg, **kwargs):
      start = time.time()
      resp = f(*args, **kwargs)
      delta = int((time.time() - start) * 1000)
      print("Handler took {delta} ms to execute")
      return resp
   return inner

It could be used like this:

class ChargePoint:
   @on("Heartbeat")
   @time_it
   def on_heartbeat(self, *args, **kwargs):
        pass

Note that this implementation of @time_it doesn't support async handlers, but async decorators are possible. A real world implementation would be a bit more complex.

Applying the decorator above has drawbacks. One might send another Call before returning the CallResult of the wrapped handler. That would cause a deadlock. This could be avoided to make inner() an async generator that yields the response rather than returning it. But that would require discipline from a user; the user can still implement wrongly that causes a deadlock. Ideally we should come up with an API tat prevents users from implementing deadlocks.

Using a decorator allows to implement middleware for individual routes. But what if you want to apply some logic to all incoming requests? Applying the same decorator to all handlers is cumbersome.

One could apply a decorator to ChargePoint._handle_call(). But that function is 'private' right now. Maybe we should change that.

@OrangeTux OrangeTux mentioned this issue Jan 26, 2021
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

1 participant