fix: add guard clause to prevent FD leak

[td-tomasz]

• Added guard clause to prevent trying to rotate files when there is already ongoing rotation
• Destroy streams in cleanup method to make sure they are "destroyed"

[td-tomasz]

FEI, generally whole issue was introduced in Node v13.0.0 (about 3 and half years ago), most likely by one of stream related changes.

[wbt]

I understand this to be an alternative to #2300 and this one also looks pretty good; does anyone else have comments/review?

Thanks @td-tomasz-joniec for the simplified PR!

(Addition of a regression test would be welcome, but I don't see the absence of one as a merge-blocker here.)

[td-krzysiek]

Thanks, we are looking forward for the update, in one of our applications (edge case) this resulted in entire drive filled up in a few hours.

For me the problematic part was related to the fact that the log function can reach this._stream.write [1] multiple times before logged [2] function is called for the first time (and _rotate flag is set). This results in multiple logged calls which is problematic. Since the code checks _rotate flag in logged function, there is no way that this._endStream(() => this._rotateFile()); is called multiple times.

• [1]

  winston/lib/winston/transports/file.js

  Line 190 in 99bc940

  const written = this._stream.write(output, logged.bind(this));

• [2]

  winston/lib/winston/transports/file.js

  Line 151 in 99bc940

  function logged() {

There was similar attempt in #2115 but I don't think this is correct, the _rotate flag is cleared too early which results in incorrect behaviour of recursive log queuing here:

winston/lib/winston/transports/file.js

Lines 134 to 140 in 99bc940

	if (this._rotate) {
	this._stream.once('rotate', () => {
	this._rotate = false;
	this.log(info, callback);
	});
	return;
	}

I hope this helps :)

[td-krzysiek]

Hello,

@wbt any news on this one?

Kind regards,
Krzysztof

[td-krzysiek]

Great, thank you! Will you be able to npm release too? This will be very helpful.

[wbt]

Done.