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

Fix timeout error handling #716

Merged
merged 3 commits into from
Nov 10, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions packages/react-server/core/ClientController.js
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,15 @@ class ClientController extends EventEmitter {
// These resolve with React elements when their data
// dependencies are fulfilled.
var elementPromises = PageUtil.standardizeElements(page.getElements());
var timeoutDfd = [];
var elementPromisesOr = elementPromises.map((promise, index) => {
var orPromise = Q.defer();
timeoutDfd[index] = Q.defer();
promise.then(orPromise.resolve)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs rejection handling, too, right?

promise.catch(orPromise.reject);

timeoutDfd[index].promise.catch(orPromise.reject);

return orPromise.promise;
});

// These resolve with DOM mount points for the elements.
//
Expand Down Expand Up @@ -672,7 +681,7 @@ class ClientController extends EventEmitter {
// Always render in order to proritize content higher in the
// page.
//
elementPromises.reduce((chain, promise, index) => chain
elementPromisesOr.reduce((chain, promise, index) => chain
.then(() => promise
.then(element => rootNodePromises[index]
.then(root => renderElement(element, root, index))
Expand All @@ -691,7 +700,14 @@ class ClientController extends EventEmitter {
// Look out for a failsafe timeout from the server on our
// first render.
if (!this._previouslyRendered){
this._failDfd.promise.then(retval.resolve);
this._failDfd.promise.then(() => {
elementPromises.forEach((promise, index) => {
//Reject any elements that have failed to render
if (promise.isPending()) {
timeoutDfd[index].reject(`Error with element ${index}, it failed to render within timeout time`);
}
});
});
}

return retval.promise.then(() => {
Expand Down
2 changes: 2 additions & 0 deletions packages/react-server/core/renderMiddleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -736,6 +736,8 @@ function writeBody(req, res, context, start, page) {

// Let the client know it's not getting any more data.
renderScriptsAsync([{ text: `__reactServerClientController.failArrival()` }], res)
// Close off the body since the page life cycle will not complete
res.write("</div></body></html>");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is enough. It's addressing the symptom (we're not sending the close tags) rather than the cause (we're short-circuiting out of the normal control flow, which includes sending the close tags).

Can we handle individual element render failures without rejecting the writeBody return promise?

});

Q.all(dfds.map(dfd => dfd.promise)).then(retval.resolve);
Expand Down