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

Always return JS engine to pool after component render. #270

Merged
merged 2 commits into from May 22, 2016

Conversation

Projects
None yet
5 participants
@dustinsoftware
Copy link
Collaborator

dustinsoftware commented May 19, 2016

This allows other threads to use available engines while other MVC actions are being processed during a page render.

Always return JS engine to pool after component render.
This allows other threads to use available engines while other MVC actions are being processed during a page render.
@huan086

This comment has been minimized.

Copy link

huan086 commented May 19, 2016

On some of my pages, I render 2 or 3 separate components (i.e. 2 or 3 Html.React calls). Some of these calls are in Html.Partial. Will this change cause any issues?

@dustinsoftware

This comment has been minimized.

Copy link
Collaborator

dustinsoftware commented May 19, 2016

Nope, you should be just fine.

@Daniel15

This comment has been minimized.

Copy link
Member

Daniel15 commented May 21, 2016

Thanks! The only issue I can think of is if someone is relying on global state - For instance, if one component's render function sets a global variable, and then another component expects that variable to be set. I'd like to think that that's a rare use-case though. And in cases like that, the user can just disable engine pooling.

Could you please add a unit test, at least for the ReturnEngineToPool method (and ideally the HTML helpers, if it's not too difficult to write tests for them)?

@Daniel15 Daniel15 merged commit 4887288 into reactjs:master May 22, 2016

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@dustinsoftware

This comment has been minimized.

Copy link
Collaborator

dustinsoftware commented May 27, 2016

FWIW we've been running this fix in production for a week now and have had no problems so far! 🎉

@mwethington

This comment has been minimized.

Copy link

mwethington commented Jul 17, 2016

dustin - how is it going after a month or so?

@dustinsoftware

This comment has been minimized.

Copy link
Collaborator

dustinsoftware commented Jul 17, 2016

Really well so far. One time one of the pooled v8 engines got in a weird
state and stopped responding - we had to recycle the app to fix it. That
has only happened one time. There is already an issue open somewhere about
that bug..

On Sun, Jul 17, 2016 at 11:15 Bill Bell notifications@github.com wrote:

dustin - how is it going after a month or so?


You are receiving this because you authored the thread.

Reply to this email directly, view it on GitHub
#270 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA5hFjf34tMKuCNkDSsG1edwOSZOMiIfks5qWnEygaJpZM4Ih63p
.

@mwethington

This comment has been minimized.

Copy link

mwethington commented Jul 17, 2016

The fix appears to be in NuGet - v2.4 - right? Can we get a blog entry for 2.5? http://reactjs.net/blog/

https://www.nuget.org/packages/React.Web.Mvc4/

@mwethington

This comment has been minimized.

Copy link

mwethington commented Jul 17, 2016

OK 2.4 is the latest - 2.5 was for Core stuff changing.

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