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

The @atomic decorator now work for class grouped handler #31

Open
thomaszdxsn opened this issue Mar 30, 2018 · 6 comments
Open

The @atomic decorator now work for class grouped handler #31

thomaszdxsn opened this issue Mar 30, 2018 · 6 comments

Comments

@thomaszdxsn
Copy link

thomaszdxsn commented Mar 30, 2018

I watch aiohttp's doc, it teach me use the aiojobs.atomic to avoid request cancel.

And I found it not work on class grouped handler, I don't mean class view, just this mean method handler:

class UserRelated(object):
     
    def create_user_handler(self, request):
        ...

I investigate the source code, ensure the @atomic decorator cause the problem.

Traceback on class grouped handler

web_protocol.py            310 ERROR    Error handling request
Traceback (most recent call last):
  File ".../lib/python3.6/site-packages/aiohttp/web_protocol.py", line 381, in start
    resp = await self._request_handler(request)
  File ".../lib/python3.6/site-packages/aiohttp/web_app.py", line 322, in _handle
    resp = await handler(request)
TypeError: wrapper() takes 1 positional argument but 2 were given

If you also think this is a bug, I can fix it

@asvetlov
Copy link
Member

Sorry, I don't see how to fix it in a generic way.
A custom decorator for instance method can be created easily though.
Or we can support @atomic(method=True) construction.

@thomaszdxsn
Copy link
Author

thomaszdxsn commented Mar 31, 2018

It is a good idea, but for support back-compat, @atomic must have both call usage and non-call usage.

or just use inspect.ismethod()?

def atomic(coro):
    @wraps(coro)
    async def wrapper(*args):
        if inspect.ismethod(coro):
            instance = args[0]
            if isinstance(instance, View):
                # Class Based View decorated.
                request = instance.request
            else:
                # Method Based View
                request = args[1]
        else:
            request = args[0]


        job = await spawn(request, coro(request))
        return await job.wait()
    return wrapper

but the code look is ugly, and more expensive.

@asvetlov
Copy link
Member

It is a good idea, but for support back-compat, @atomic must have both call usage and non-call usage.

Yes.

but the code look is ugly, and more expensive.

That's why I did not support classes from very beginning (and still not sure what way is better now).

@thomaszdxsn
Copy link
Author

Ok.

We should postpone this issue if not have better idea.

But maybe we can add some doc for illustrate?

@asvetlov
Copy link
Member

asvetlov commented Apr 1, 2018

Doc example sounds perfect.

@brawer
Copy link

brawer commented Jan 23, 2019

Would the approach of https://github.com/nkoshell/aiojobs/commit/20d7a34a74b2fb50a968f875d704ea0279a5ae7e be reasonable? Just wondering because I’ve run into this same problem, but I don’t have enough Python fu to pick up the (apparently abandoned?) work myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants