Respect termination signals

[TiloW]

clcache ignores termination signals.
This is bad because a single call to cl may take rather long. For example, when using large header-only libraries or invoking the compiler for many source files at once.

[frerich]

The code was originally merged as part of #233, which addresses a real issue. Maybe a compromise would be to adjust the code such that the signals are only ignored while the cache is modified?

[TiloW]

a compromise would be to adjust the code such that the signals are only ignored while the cache is modified

Either that or handle corrupt cache entries correctly, i.e., invalidate them upon lookup.
More precisely, these signals should never be ignored but one might decide to wait for the cache modification to finish until exiting the program.

[siu]

@TiloW it was important for us to ignore the signals as it was too common that we aborted compilation with ctrl-c and the cache would be corrupted.

A possible improvement is to limit the time that the signals are ignored. In the end what really needs to be protected are the operations that persist data to disk:

• copyOrLink of the compiler artifacts
• writing manifest entries
• writing the statistics
• what else?

[frerich]

How about a context manager for this, e.g. (untested)

@contextlib.contextmanager
def blockedSignals(signals=None):
    signals = signals or [signal.SIGINT, signal.SIGTERM]

    handlers = [signal.signal(sig, signal.SIG_IGN) for sig in signals]
    try:
        yield
    finally:
        for sig, handler in zip(signals, handlers):
            signal.signal(sig, handler)

With this at hand, one could wrap relevant sections of code as:

with blockedSignals():
    doWriteData()
    doWriteMoreData()
    ...

[siu]

Looks like a good solution and it doesn't make the code that uses it much harder to understand. 👍

[TiloW]

it was important for us to ignore the signals as it was too common that we aborted compilation with ctrl-c and the cache would be corrupted

This is indeed a problem. We're running builds that take several minutes and call the compiler exactly twice. So basically, once started, we can't cancel the build. Additionally, there is the fact that the OS might decide to terminate the compiler anyways (for whatever reasons). It would be nice to have a solution that is able to handle such cases as well, i.e., recognize corrupt cache files.

[TiloW]

Would it be possible do add some sort of zero bytes file that is created only after all modifications to the respective cache entry are done and deleted prior to modifying anything? So that we can easily check, just by the existence oft this file, whether we have in fact a valid cache entry.

[frerich]

Would it be possible do add some sort of zero bytes file that is created only after all modifications to the respective cache entry are done and deleted prior to modifying anything?

That's an interesting idea. Another (crazy?) idea which came up some time ago was to rename the cache directory while the entry is being edited, e.g. by appending -dirty prefix or so.

For what it's worth, I'm personally more concerned about the question: in case we notice that a cache entry is invalid, what to do about it? After all, an invalid entry in the compiler artifact repository might be referenced by a lot of manifests, too. So there's some sort of "rippling effect".

[frerich]

In any case, I think this is not an either-or thing: blocking signals only while changes are written to disk probably makes sense, and is in scope for this ticket. Making clcache handle broken caches more gracefully would be a topic for a separate ticket.

[TiloW]

blocking signals only while changes are written to disk probably makes sense, and is in scope for this ticket

Certainly an improvement but doesn't resolve this issue as the signals are still ignored (sometimes).

in case we notice that a cache entry is invalid, what to do about it?

I'm probably missing something here: why can't we just re-compile it?

[frerich]

The problem you describe is not that the signals are ignored, is it? The issue is that they are ignored for most of the lifetime of the clcache process, and that issue manifests as a problem for your use case because clcache is invoked with a lot of source files and thus runs for a long time - sounds accurate?

If so, then I imagine that merely shortening the phases during which the signals are blocked would resolve the problem, no?

[frerich]

I'm probably missing something here: why can't we just re-compile it?

Maybe we can - the code would just need to detect all cases where something could have gone wrong (say: corrupt JSON files or missing object files or so) and then signal some 'malformed cache entry' condition. And when handling that condition, the code should not break because there are (at least partially) existing files already.

[TiloW]

I imagine that merely shortening the phases during which the signals are blocked would resolve the problem

Actually, I don't know whether that would be sufficient for my use case. We're using clcache for CI with gitlab multi runner and sometimes there are certain builds that need to be canceled. I don't know (and don't want to make any assumption about) how cancellation is implemented. We don't know whether a second SIGTERM is fired if the first one did not succeed (so the change proposed above could be problematic) nor whether the OS simply terminates the process. Corrupting the cache during cancellation is obviously bad.

In conclusion, a better title for this issue would have been "Prevent cache corruption". Should I open another one? Even though I believe that when speaking about robust solutions this boils down to the same issue.

[frerich]

In conclusion, a better title for this issue would have been "Prevent cache corruption". Should I open another one?

Indeed, as I mentioned earlier - I don't think these are mutually exclusive options. It would be great to make clcache try hard to avoid writing out broken cache data (e.g. by ignoring SIGTERM and SIGINT in certain phases) and make it handle defective caches gracefully. After all, a cache may have become corrupted by external sources.

So a second ticket would make perfect sense to me, but I probably wouldn't call it "Prevent cache corruption" -- because we cannot prevent it. Instead, it would be more like "Gracefully handle corrupt caches". Sounds good?