gaminggamedevprogrammingprojectsmatrixdevGameArtleveldesignemulationTop Subs
7

I don't think most would classify it as a bug in NodeJS, but it is certainly an inconvenience. If somehow headers get sent twice or content gets sent twice to the same response object, you end up with an error that doesn't behave like most errors. You also get a stack trace for the wrong section of code.

I fixed that.

First let's reproduce the problem.

function a(res) {
 res.end('Hello');
}

function b(res) {
 res.end('World');
}

require('http')((req,res)=>{
 a(res);
 b(res);
}).listen(8080);

It's an over-simple server. But it's most important feature is it has problems. b() is calling res.end() again. But the stream was already written to and closed by a(). A common scenario where you would have a similar structure to this is if a() is meant to branch with some condition, send alternate content to the user, and the prevent logic from continuing to b().

That is to say that the error is not really in b(). The problem is a() called end and didn't stop progression. This is one of the few weak points of async/await. I'm a huge fan of async/await over callback hell. But the one strength of callbacks was that each function had to explicitly progress the next stage, usually by calling cb(). With callbacks you had three eventual progressions. Continue, error, don't progress. Really there are four when you consider that you have both cb(error) and throw new Error(), which will route the error to different places.

With async await you only have continue or error. This gives the a() function a lot less agency to do the right thing, and now the function that called a() has to understand what kind of branching can happen. Regardless figuring out how to not produces these kinds of errors is 100% possible and async/await is completely worth this weight. But still, it is nice to get good diagnostics.

The other additional issue that has nothing to do with async/await vs callbacks is related to node's asynchronous IO. Though it's hardly an excuse. When the call of end() in b() eventually throws, it doesn't do it synchronously with b(). Though it could, and frankly should.

This means we can't do this to prevent a server from crashing.

function b(res) {
 try {
  res.end('World');
 }
 catch(e) {
  //Please don't crash
 }
}

It also means we can't do other more correct things in response to the error. If you want custom logic for this case, aka what exception handling if for, you have to detect if the stream is writable before writing to it. That checks it synchronously, and proves the point that node could have thrown synchronously.

So I wrote a little module that makes it fail synchronously and reports a() in the stack and all other stacks that competitively interacted with the object.

upgradehttp.js

var http = require('http');
var prot = http.ServerResponse.prototype;
var oend = prot.end;
var owrite = prot.write;
var owriteHead = prot.writeHead;


function newend() {
 console.log('new end');
 this.writers||=[];
 this.writers.push(new Error('sibling end').stack);
 try {
  if(this.writableEnded) throw new Error('ERR_STREAM_WRITE_AFTER_END: res.end called multiple times');
  oend.apply(this,arguments);
 }
 catch(e) {
  console.log('caught new end');
  console.error(this.writers);
  throw e;
 }
}

function newwrite() {
 this.writers||=[];
 this.writers.push(new Error('sibling write').stack);
 try {
  if(this.writableEnded) throw new Error('ERR_STREAM_WRITE_AFTER_END: res.write called after res.end');
  owrite.apply(this,arguments);
 }
 catch(e) {
  console.error(this.writers);
  throw e;
 }
}

function newwriteHead() {
 this.writers||=[];
 this.writers.push(new Error('sibling writeHead').stack);
 try {
  if(this.headersSent) throw new Error('ERR_HTTP_HEADERS_SENT: res.writeHead called after headers sent');
  owriteHead.apply(this,arguments);
 }
 catch(e) {
  console.error(this.writers);
  throw e;
 }
}

prot.end=newend;
prot.write=newwrite;
prot.writeHead=newwriteHead;

Reading the code suggests that this is for the http module, but it covers https as well because the https module makes use of the http.ServerResponse class.

Now your errors are synchronous, meaning you can let your server crash or not with standard exception handling. And now you get logging of the equivalent a() function in your code. If you want this logging to go somewhere else it wouldn't be hard to change the logic. With vibe coding being on the rise, I see AI produce this exact bug all the time. So now you can detect it and have the information you need to fix it.

Without that information, if you tell AI that there is a problem with crashes in b() then it will try to fix the code in b(), likely using a if(res.writableEnded) bandaid that will prevent the crash but not fix the issues in a(). Or maybe it will figure it out. It's 50:50. But with this kind of diagnostic you can guarentee it figures out that it needs to think about the interactions between a() and its caller(s).

Yes, thanks to the loss of callbacks as a standard we now need more logic in all functions in the call stack before a() until we meet the one that's common with b(), and we need to do this for all potential callers of a(). I'm not saying that callback hell was good. But I am saying that functional programming tends to make blocks of code much more autonomous.

Comment preview