Deprecating req.flush() has caused other problems

[patrick-steele-idem]

tldr; Implementing req.flush() and having it log a deprecation message to the console is causing problems with code that expects flush() to be implemented correctly.

I understand that OutgoingMessage used to have a flush() method that was poorly named. This was later fixed by renaming it to flushHeaders(). However, instead of stopping there, a stub for the flush() method was put in with the following implementation:

OutgoingMessage.prototype.flush = util.deprecate(function() {
    this.flushHeaders();
}, 'flush is deprecated. Use flushHeaders instead.');

See commit: b2e00e3

I understand that this was done to maintain backwards compatibility, but it is also preventing libraries from actually using the flush() method for the correct purpose. It's my opinion that we should have just deleted the old flush() method (it was a major release after all), but it's a little late for that now....

This change is causing a problem for libraries that check if flush() is implemented and want to use it for the purpose of actually flushing buffered data. You are probably thinking that flush() was never used for that purpose, but that is not true...For example, the compression module monkey patches the response stream to include a flush() method for the expected and correct purpose. See:

https://github.com/expressjs/compression/blob/c2af8bd8d5cec3577b40d61859ca3a0467052ded/index.js#L61-L65

The marko templating engine is an asynchronous and streaming templating engine that is designed to flush() bytes to the underlying stream whenever an asynchronous fragment is completed. It works by conditionally checking if the underlying stream has a flush() method and it will then call the flush() method if found. See:

https://github.com/marko-js/async-writer/blob/5ef039139041fb40aadcdc0b7a10eb7d3d3a48b5/lib/AsyncWriter.js#L494-L496

This works great when using the gzip compression middleware that monkey patches the flush() method but when rendering to an OutgoingMessage stream with the compression middleware we are seeing a deprecation message being written to the console 😞

As a result, users of marko are reporting the problem that a deprecation warning is being written to the console: marko-js-archive/async-writer#3

I understand the dilemma, but without knowing if a stream is really flushable we are seeing problems when trying to use the flush() method for the purpose that the compression middleware intended.

I don't know the right answer here, but just wanted to start the discussion to see how soon we can fix this inconsistency (Node.js 5.0? If so, how long will that be?) and to make more people aware of the issue. Is there a lot of code in the wild that was using flush() for the purpose of flushing headers on an OutgoingMessage?

[patrick-steele-idem]

Also, I just wanted to add that after digging in the history some more I realized that Node.js never had OutgoingMessage.prototype.flush (just io.js). I thought maybe it was added in Node.js 0.12 but that is not the case. Given that OutgoingMessage.prototype.flush only ever existed in io.js it feels even more sad that flush() has been ruined for Node.js 4.0 :(

[Fishrock123]

I'm going to move to close this. This is how we deprecate things. You can use --no-deprecation (I think that's the right one) if you don't want warnings printed.

[patrick-steele-idem]

Now that someone has committed the bad code I agree with you it has to be deprecated properly but it is still causing a problem. ..

The problem is that the original addition of OutgoingMessage.prototype.flush introduced both req.flush() and res.flush(). Long before Node.js introduced the flush() method, the popular compression middleware was already introducing a res.flush() method and now we have a conflict since we have the same method for different purposes. I realize that issues caused by monkey-patching in third-party libraries is not something the Node.js core team should have to deal with which is why I asked the following question:

I don't know the right answer here, but just wanted to start the discussion to see how soon we can fix this inconsistency (Node.js 5.0? If so, how long will that be?) and to make more people aware of the issue. Is there a lot of code in the wild that was using flush() for the purpose of flushing headers on an OutgoingMessage?

I would like to see the OutgoingMessage.prototype.flush be completely removed or to make it a no-op. For now, the workaround is to do the following:

OutgoingMessage.prototype.flush = function() {}
// Or: delete OutgoingMessage.prototype.flush;

...And hope that any code has already moved to flushHeaders() :)

We have to put in a hack to solve the problem before we can move to Node.js 4.x because of a problem that was introduced in io.js before the merge. Definitely not a major issue though. I'm okay with you closing this issue, but do please let me know when we could expect the deprecated OutgoingMessage.prototype.flush to be removed. Thanks for reading and I apologize for the long text.

[Fishrock123]

Here is the code:

res.flush: https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L675-L686

util.deprecate: https://github.com/nodejs/node/blob/master/lib/internal/util.js

This just means the library you are using hasn't been updated yet. We've long issues deprecation warning this way and will continue to. If printing to stdio causes issues for you, you need to run node with --no-deprecation.

The property will likely be deleted in a year or two, for now, just check the node version.

As for the original addition, that well over a year ago in 0.11.14: bd24ab2. Not much we can do in retrospect.