Don't create the stream eagerly in lock(); that may cause JGit to
exceed OS or JVM limits on open file descriptors if many locks need
to be created, for instance when creating many refs. Instead create
the output stream only when one really needs to write something.
Bug: 573328
Change-Id: If9441ed40494d46f594a896d34a5c4f56f91ebf4
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
In a distributed setting, one can have multiple datacenters use
reftables for serving, while the ground truth for the Ref database is
administered centrally. In this setting, replication delays combined
with compaction can cause update-index ranges to overlap.
Such a setting is used at Google, and the JGit code already handles
this correctly (modulo a bugfix that applied in change I8f8215b99a).
Remove the restriction that was applied at FileReftableDatabase.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Change-Id: I6f9ed0fbd7fbc5220083ab808b22a909215f13a9
We are running several servers with jGit. We need to run repack from
time to time to keep the repos performant. I.e. after push we test how
many small packs are in the repo and when a threshold is reached we run
the repack.
After upgrading jGit version we've found that if someone does the clone
at the time repack is running the clone sometimes (not always) fails
because the repack removes .pack file used by the clone. Server
exception and client error attached.
I've tracked down the cause and it seems to be introduced between jGit
5.2 (which we upgraded from) and 5.3 and being caused by this commit:
Move throw of PackInvalidException outside the catch -
afef866a44
The problem is that when the throw was inside of the try block the last
catch block catched the exception and called openFailed(false) method.
It is true that it called it with invalidate = false, which is wrong.
The real problem though is that with the throw outside of the try block
the openFail is not called at all and the fields activeWindows and
activeCopyRawData are not set to 0. Which affects the later called tests
like: if (++activeCopyRawData == 1 && activeWindows == 0).
The fix for this is relatively simple keeping the throw outside of the
try block and still having the invalid field set to true. I did
exhaustive testing of the change running concurrent clones and pushes
indefinitely and with the patch applied it never fails while without the
patch it takes relatively short to get the error.
See: https://www.eclipse.org/lists/jgit-dev/msg04014.html
Bug: 569349
Change-Id: I9dbf8801c8d3131955ad7124f42b62095d96da54
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
- log an error
- either there is no list or it is incomplete hence return immediately
Change-Id: Ieee5378ca06304056b9ccc30c1acd5f52360052d
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
If pack or index files are guarded by a pack lock (.keep file)
deleteOrphans() should not touch the respective files protected by the
lock file. Otherwise it may interfere with PackInserter concurrently
inserting a new pack file and its index.
The problem was caused by the following race.
All mentioned files are located in "objects/pack/".
File endings relevant in "pack" dir:
.pack
.keep
.idx
.bitmap
When ReceivePack receives a pack file it executes the following steps:
ReceivePack.service():
receivePackAndCheckConnectivity():
receivePack():
receive the pack
parse the pack, returns packLock (.keep file)
PackInserter.flush():
write tmpPck file: "insert_<random>.pack"
write tmpIdx file: "insert_<random>.idx"
real pack name: "pack-<SHA1>.pack"
real index name: "pack-<SHA1>.idx"
atomic rename tmpPack to realPack
atomic rename tmpIdx to tmpIdx
execute commands
unlock pack by removing .keep file
trigger auto gc if enabled
When PackInserter.flush() renames the temporary pack to the final
"pack-xxx.pack" file the temporary pack index file "insert_xxx.idx"
has no matching .pack file with the same base name for a short interval.
If deleteOrphans() ran during that interval it deduced the pack index
file was orphaned. Subsequently the missing pack index caused
MissingObjectExceptions since objects contained in the pack couldn't be
looked up anymore.
Bug: https://bugs.chromium.org/p/gerrit/issues/detail?id=13544
Change-Id: I559c81e4b1d7c487f92a751bd78b987d32c98719
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
Just allocate the two string objects directly. The previously used
new StringBuilder(0).toString() returns the same object for both END
and DELIM when run on Java 15, which breaks the wire protocol since
then END and DELIM cannot be distinguished anymore.
Bug: 568950
Change-Id: I9d54d7bf484948c24b51a094256bd9d38b085f35
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
(cherry picked from commit 7da0f0a8f3)
The current mechanism for updating the unpack error handler requires
that the error handler is replaced entirely, including communicating
the error to the user. Adding a getter means that delegating
implementations can be constructed so that the error can be processed
before sending to the user, for example for logging.
Change-Id: I4b6f78a041d0f6f5b4076a9a5781565ca3857817
Signed-off-by: Jack Wickham <jwickham@palantir.com>
* stable-5.7:
ObjectDirectoryInserter: Open FileOutputStream in try-with-resource
ObjectDirectoryInserter: Remove redundant 'throws' declarations
ObjectDirectory: Further clean up insertUnpackedObject
ObjectDirectory: Explicitly handle NoSuchFileException
ObjectDirectory: Fail immediately when atomic move is not supported
Change-Id: I05186baa517388680fcc6825c940c4c772f26d32
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
ObjectWritingException and FileNotFoundException are subclasses
of IOException, which is already declared. Error does not need
to be explicitly declared.
Change-Id: I879820a33e10ec3a7ef676adc9c9148d2b3c4b27
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
- The code to move the file is repeated. Split it out into a
utility method.
- Remove the catch block for AtomicMoveNotSupportedException which
is redundant because it's handled in exactly the same way as the
IOException further down. The only exception we need to explicitly
handle differently in this block is NoSuchFileException.
- Improve the comments.
Change-Id: Ifc5490953ffb25ecd1c48a06289eccb3f19910c6
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
The shutdown method releases
- ThreadLocal held by NLS
- GlobalBundleCache used by NLS
- Executor held by WorkQueue
Bug: 437855
Bug: 550529
Change-Id: Icfdccd63668ca90c730ee47a52a17dbd58695ada
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
If a hunk does not apply at the position stated in the hunk header
try to determine its position using the old lines (context and
deleted lines).
This is still a far cry from a full git apply: it doesn't do binary
patches, it doesn't handle git's whitespace options, and it's perhaps
not the fastest on big patches. C git hashes the lines and uses these
hashes to speed up matching hunks (and to do its whitespace magic).
Bug: 562348
Change-Id: Id0796bba059d84e648769d5896f497fde0b787dd
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
Running recent error prone version complaining on that code:
CharacterHead.java:22: error: [ProtectedMembersInFinalClass] Make
members of final classes package-private: <init>
protected CharacterHead(char expectedCharacter) {
^
(see https://errorprone.info/bugpattern/ProtectedMembersInFinalClass)
Did you mean 'CharacterHead(char expectedCharacter) {'
Bug: 562756
Change-Id: Ic46a0b07e46235592f6e63db631f583303420b73
Signed-off-by: David Ostrovsky <david@ostrovsky.org>
On the first attempt to move the temp file, NoSuchFileException can
be raised if the destination folder does not exist. Instead of handling
this implicitly in the catch of IOException and then continuing to
create the destination folder and try again, explicitly catch it and
create the destination folder. If any other IOException occurs, treat
it as an unexpected error and return FAILURE.
Subsequently, on the second attempt to move the temp file, if ANY kind
of IOException occurs, also consider this an unexpected error and
return FAILURE.
In both catch blocks for IOException, add logging at ERROR level.
Change-Id: I9de9ee3d2b368be36e02ee1c0daf8e844f7e46c8
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
If atomic move is not supported, AtomicMoveNotSupportedException will
be thrown on the first attempt to move the temp file. There is no
point attempting the move operation a second time because it will only
fail for the same reason.
Add an immediate return of FAILURE on the first occasion. Remove the
unnecessary handling of the exception in the second block.
Change-Id: I4658a8b37cfec2d7ef0217c8346e512968d0964c
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
Running recent error prone version complaining on that code:
RefDatabase.java:444: error: [InvalidInlineTag] Tag name `linkObjectId`
is unknown.
* Includes peeled {@linkObjectId}s. This is the inverse lookup of
^
(see https://errorprone.info/bugpattern/InvalidInlineTag)
Bug: 562756
Change-Id: If91da51d5138fb753c0550eeeb9e3883a394123d
Signed-off-by: David Ostrovsky <david@ostrovsky.org>
Matthias Sohn