tree b64399f479cafa1012fea8fa0cc7532a52cec7b3
parent fa6336bb08a293c61d9c2c5f96b6193cefd72469
author Patrick Hiesel <hiesel@google.com> 1649316746 +0200
committer Patrick Hiesel <hiesel@google.com> 1649681084 +0200

Fix threading issue in Diff cache

We have observed occasional failures in computing diffs. The
computation fails with a Zlib exception making it look like
the pack file is currupted. Fsck, however, tells that the
pack file is fine.

Upon closer investigation, it seems like there is a case
in the diff cache were we process on a different thread
and cancel in case of a timeout. The objects we pass
around (ObjectReader and DiffFormatter) are not thread safe.

The JavaDoc of ObjectReader states that it is not thread
safe and the Dfs implementation restates that. Checking
the implementation also shows that it's not thread safe
since it uses member variables to store intermediary
results. Other implementations might be thread safe, but
if the abstract class doesn't guarantee that, we can't
rely on it.

In Java, a timeout of a future does not guarantee to
have actually terminated the work item when it occurs. So
there could be a situation where the work item has timed out,
but we are still processing anyway. The main thread then
continues to process a different diff in a different region
of the pack. The reused ObjectReader then gets confused as
it races to read different parts of the pack.

This is an attempt to fix the problem by using a pool to
share DiffFormatter instances. DiffFormatters that are closed
will be returned to the pool. If the pool itself is already
closed, the a close call to the DiffFormatter's wrapper
(Handle) will propagate to the DiffFormatter and consequently
the ObjectReader). Closing the pool will close any resources
currently in the pool.

We run into this problem, because the diff cache uses getAll
which sequentially loads many diffs for a single request.

Google-Bug-Id: b/209491483
Release-Notes: Fix threading issue in diff cache
Change-Id: I51bd1a73f573feab0eb58f825f28024852c17e47