jbuck / express-persona

Mozilla Persona integration for Express
http://jbuck.github.com/express-persona
BSD 2-Clause "Simplified" License
66 stars 16 forks source link

trying to set headers after being sent #4

Closed clux closed 11 years ago

clux commented 11 years ago

got a crash with the above, moving the headers to the top seemed to work for me. https.request docs suggested putting .end() before error handlers also.

anyway, thanks for this module, made the process a lot easier :) btw, mozilla now suggests using navigator.id.watch for doing the client side xhr.

jbuck commented 11 years ago

Can you post the stack trace you get when using the current version of express-persona? And your version of node too. I'd love to figure out why the current code breaks for you, but changing the ordering fixes it.

Yeah, I should switch the README example to use the 3rd-gen API. I filed issue #5 for that.

clux commented 11 years ago

I'll try to get a trace tomorrow. I'm on node 0.8.11.

clux commented 11 years ago

Sorry, you were right to be sceptical on this one. Re-reading the traces shows I was mis-attributing one 'fix' for what was actually something completely different.

Basically: crash 1 because missing bodyParser causing a cluster worker to handle the error event which then triggered the header crash (which was actually in a completely different place).

Missing body parser crash:

TypeError: Cannot read property 'assertion' of undefined at module.exports.app.post.req.session.(anonymous function) (/home/clux/repos/deathmatch/node_modules/express-persona/index.js:90:26) at callbacks (/home/clux/repos/deathmatch/node_modules/express/lib/router/index.js:162:11) at param (/home/clux/repos/deathmatch/node_modules/express/lib/router/index.js:136:11)

Headers crash from error handler being picked up from a worker. http.js:644 throw new Error('Can\'t set headers after they are sent.'); ^ Error: Can't set headers after they are sent. at ServerResponse.OutgoingMessage.setHeader (http.js:644:11) at ServerResponse.res.setHeader (/home/clux/repos/deathmatch/node_modules/express/node_modules/connect/lib/patch.js:59:22) at ServerResponse.res.set.res.header (/home/clux/repos/deathmatch/node_modules/express/lib/response.js:516:10) at ServerResponse.res.json (/home/clux/repos/deathmatch/node_modules/express/lib/response.js:193:8) at Object.defaultOptions.verifyResponse (/home/clux/repos/deathmatch/node_modules/express-persona/index.js:19:9) at ClientRequest.module.exports.app.post.JSON.stringify.assertion (/home/clux/repos/deathmatch/node_modules/express-persona/index.js:86:19) at ClientRequest.EventEmitter.emit (events.js:93:17) at CleartextStream.socketCloseListener (http.js:1314:9) at CleartextStream.EventEmitter.emit (events.js:123:20) at SecurePair.destroy (tls.js:938:22) 09:27:57 - WARN - worker 8923 died

This can probably be ignored safely. Sorry to waste your time on this, was clearly not in the zone yesterday D:

jbuck commented 11 years ago

I filed issue #6 to handle a missing bodyParser. At the very least, express-persona shouldn't die.

For your headers already being sent crash, you can pass your own verifyResponse function that can attach custom headers to the response:

verifyResponse: function(error, req, res, email) {
  var out;
  if (error) {
    out = { status: "failure", reason: error };
  } else {
    out = { status: "okay", email: email };
  }
  res.header("X-Custom-Header", "hello world");
  res.json(out);
}

I don't think this was a waste of time, I'll be able to make the module better :)

clux commented 11 years ago

Hehe good : ) Yeah, I used the default verifyResponse when the crash occurred. Curious how the error event leaked over to another cluster worker at any rate.