As reported by Jani M. :
One further change that I would like to propose involves a race condition in
the graph generation. Currently, it is possible that two (or more) CGI
processes would concurrently try to update the same graph.
This can obviously fail in quite many ways, and I suspect I already see this
happening on our setup. A few random graph loads have failed, with just the
" Warning: Could not draw graph ..." error.
I'd propose changing the design so that RRDs::graph would write to a
temporary file, and if successful, only then the result would be move()'d to
the final filename. This should avoid multiple processes writing to the same
file.
And while on the topic of race conditions, I think I spotted a second one
too. It is possible that in between the "is the graph fresh enough check",
and actually sending the data to the client, for a second CGI process to
fail the freshness-check, and unlink() the file before the first process
gets around to sending the content.
This should be quite rare, hopefully, but possible none the less. A possible
cure could be to simply remove the unlink. Combined with the above "generate
graph to temporary file" change, just overwrite the graph file with the new
when graph generation is complete -- if it fails, you can serve the old
graph.
I would prefer to lock the output file, this way the racing is avoided, and 2 same calls are serialised. The first creates the graph and serves it, the second one just serves it, without generating it. This behaviour saves some CPU & IO :-)
