haraka / haraka-config

Haraka config file loader and parser
https://www.npmjs.com/package/haraka-config
MIT License
10 stars 13 forks source link

Changing or disabling timeout for config reload? #42

Closed analogic closed 6 years ago

analogic commented 6 years ago

I see there is 5 * 1000 timeout for reloading configs (https://github.com/haraka/haraka-config/blob/f501adc4d3a288d062e979c0b2fe86d202e5894c/configfile.js#L130) - I've written some integration tests and time adds up, is there any particular reason for such long time? Is it bad idea to switch off waiting entirely?

smfreegard commented 6 years ago

Unless I'm missing something - that's a 5 second timer.

IIRC - the delay was introduced so that we don't do a mass amount of re-reads of the same file when one of the files is touched (because we get multiple events per file, so sedate the reads to combine them all into one, 5 seconds after the last event).

You can run without the timer, but it means you add unnecessary reloads on OSs where we get multiple events for the same file and that can cause a CPU spike and is very undesirable if the file being re-read is large.

Can you be a bit more specific as to why waiting for 5 seconds is a problem? Or do you mean you're doing lots of integration tests and because of the 5 second wait - they take a while for all of them to complete?

If so, then I don't think it would be desirable to remove this for the testing, it needs to work like it would in production otherwise we aren't testing like-for-like.

msimerson commented 6 years ago

The root issue was identified when I was working on a mail system that wrote out a rather large config file consisting of ~100,000 domains that the mail server was accepting mail for. Since it took a bit of time to write the file, Haraka would get a file change event well before the file was finished writing. Haraka would then re-read the partially written file with obvious negative effect.

I worked around that problem on that mail system by having the file written to disk as a tmp file, and then moving the temp file into place. We added the 5 second delay to prevent such an issue from harming other users.

If you're certain your use of Haraka won't be harmed by removing that, feel free to make the change locally. There's may be another way to solve the "detect that a file is no longer being written to, and thus safe to re-read" issue, but one hasn't sprung to mind.

analogic commented 6 years ago

@smfreegard yes, I am doing lot of tests - currently I restart node on every settings change, but it is not "cheap" nor time predictable on different setups. Simulate Haraka in production and test it with all feature set is almost impossible due lot of external variables, so I am ok with some kind of tweaking...

@msimerson Thanks, I thought it is some kind of protection. (I very much like "incron" which works similary and I know that it can get easily messed up)

I am afraid that "detect that a file is no longer being written to" will not work on different platforms. I would like to contribute in some way, but the best solution I've come to was monkey patch this file to fit my need