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

Already on GitHub? Sign in to your account

Dancer is not compatible with Corona #929

Closed
berekuk opened this Issue May 27, 2013 · 14 comments

Comments

Projects
None yet
5 participants
Contributor

berekuk commented May 27, 2013

But it recommends it as a deployment option.

How to reproduce: https://gist.github.com/berekuk/5657189

Result I observed from this gist: thread1 prints "bar…bar…bar…" until I run the thread2 in another console, and then thread1 starts printing "foo…foo…", while thread2 prints "bar…bar…bar…"

PS: It's reproducible both with Dancer and Dancer2.

Contributor

yanick commented May 28, 2013

Okay, that is funky...

I'll have to look at it a mite closer. But as far as I know, Dancer
doesn't do anything special with any of the psgi servers. So I just
wonder how much Corona itself is resilient to random acts of Coro'ing
done within the application...

But yeah, real intriguing. Thanks for the report!

Joy,
`/anick

tehmoth commented May 28, 2013

Equivalent code using Web::Simple under Corona does the right thing. Kelp seems to fail worse though.

This gist:
https://gist.github.com/tehmoth/99ed569b26e0296ddcbf (run with plackup -s Corona app.pl
when connected to with just GET hello/foo:
while true; do wget -q -O - 'http://localhost:5000/hello/foo';echo; done
You get lines like
/hello/foo:/hello/foo:foo:foo:8812944544:8812944544:8662921496:8662921496
/hello/foo:/hello/foo:foo:foo:8784741688:8784741688:8758651848:8758651848
/hello/foo:/hello/foo:foo:foo:8812944232:8812944232:8662922792:8662922792
Running GET /hello/bar at the same time causes the first script (foo) to return lines like:
/hello/foo:/hello/bar:foo:bar:8662921304:8702733328:8758649928:8662921592
/hello/foo:/hello/bar:foo:bar:8617719792:8662239616:9256666880:8678678616
/hello/foo:/hello/bar:foo:bar:8617718352:8758649712:8758652112:8662924304

and the other command (bar):
/hello/bar:/hello/foo:bar:foo:8702733328:8617719792:8662921592:9256666880
/hello/bar:/hello/foo:bar:foo:8662239616:8617718352:8678678616:8758652112
/hello/bar:/hello/foo:bar:foo:8758649712:8617720224:8662924304:8833652048

Notice how it seems to have re-used the request() (and the params()) object

Contributor

berekuk commented May 28, 2013

@yanick, it's not really that surprising. Coro threads share everything by default, and Dancer APIs are not object-oriented, and so don't have any means to track the right context when you enter two routes at the same time.

See: https://gist.github.com/berekuk/5661492

The only solution I can see is to track multiple Dancer::SharedData instances in a hashref, identified by Coro's thread_id.
It's either this or change the entire API to the explicit $context->param(...), $context->var(...), etc.
Or, give up on Coro support and change the docs :)

tehmoth commented May 28, 2013

Berekuk: I thought one of the points of Dancer2 was the removal of Shared Data. It should be possible for functions like request and params to access data local to the current scope without relying on Coro thread_ids or converting to an explicitly object oriented API.

Contributor

berekuk commented May 28, 2013

I don't see how that's possible. I looked into Dancer2 code, and it looks like request is stored in app->context->request, so it's still one request per app, and you usually have only one app.

How do you suggest param() to access the right context, when it's just a function and not a method? If it were a closure, I could see how it could refer to the private, per-request variable, but it's not, it's exported just once when you start your app.

@yanick yanick added a commit that referenced this issue Jul 21, 2013

@yanick yanick provide warning for Corona in pod
See issue #929
63deafe
Contributor

yanick commented Jul 21, 2013

@berekuk Does the caveat given at 63deafe sounds sane to you?

Contributor

berekuk commented Jul 21, 2013

I'd prefer more strong wording, I think. More like "Don't even think about using Corona or any other AnyEvent-based server". In all caps, MST-style.

"Can cause" implies that there's a chance it'll be ok. So someone can decide that their app is safe, because they checked it in the browser and it looked like it's working. But it really, really won't. This issue will come up months later in production, and it will be hard to reproduce. In the worst case, users will get the personal data from other users, something they were never meant to see (so this can turn into a big security issue).

Maybe Dancer should just detect AnyEvent presence and die with fatal error.

Contributor

yanick commented Jul 22, 2013

"Can cause" implies that there's a chance it'll be ok.

The way I understand it, it will be okay as long as the application itself doesn't use any AnyEvent sleeps like the one you have in the gist. If the user's code don't explicitly use AnyEvent or Coro, a request can't yield to another one until it's done, and thus everything should be fine. Or am I missing one of the problematic cases?

Owner

sukria commented Jul 22, 2013

sent on the way, short email!
Le 28 mai 2013 13:18, "Vyacheslav Matyukhin" notifications@github.com a
écrit :

I don't see how that's possible. I looked into Dancer2 code, and it looks
like request is stored in app->context->request, so it's still one request
per app, and you usually have only one app.

This is wrong. Dancer 2 creates one context object per request.

So Dancer 2 should behave nicely here and was designed exactly to fix that.

I suggest doing the same tests with a dancer 2 app.

How do you suggest param() to access the right context, when it's just a
function and not a method? If it were a closure, I could see how it could
refer to the private, per-request variable, but it's not, it's exported
just once when you start your app.

You are describing here the major difference between 1 and 2.

Anything related to D1 globals issues should point to D2.

This is the case here.

Contributor

berekuk commented Jul 22, 2013

@yanick, you're right, but there's no reason to using Corona in this case!
Also, it's possible to unintentionally yield by using a module which uses Coro internally.

@sukria, but I did, the test case is right there in my initial issue description.

Owner

sukria commented Jul 22, 2013

My bad, then could you report a Bug (it is a bug) against Dancer2's repository?

Thanks.

This can't work properly on D1, but should on D2.

@berekuk berekuk referenced this issue in PerlDancer/Dancer2 Jul 22, 2013

Closed

Corona compatibility #324

Owner

ambs commented Jul 25, 2013

@sukria , when you say it can't work properly on D1, means we should close this issue, or that it should work badly with D1? :)

Contributor

yanick commented Jul 25, 2013

@ambs It means we should document the limitations of D1 w/ Corona in the POD, and close the issue. I'm working on the right wording for the documentation as we speak. :-)

@yanick yanick added a commit that referenced this issue Jul 27, 2013

@yanick yanick provide warning for Corona in pod
See issue #929
4513728
Contributor

yanick commented Jul 27, 2013

Documentation changed with stern warning. :-)

@yanick yanick closed this Jul 27, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment