Add locking and traps for gtags_update.sh, and really do locking #361
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Locking is good to have for all the user's databases, not just ctags.
We already have this with ctags, should have it for gtags too.
This argument is a path where tags should go (ie g:gutentags_cache_dir, or the directory to scan if not set) and would need to be quoted if it had spaces or other characters meaningful to the shell. The variable gets assessed by 'eval' before being used to invoke gtags, so we want to embed the escapes within the string. While we do this for correctness, note that unix cscope cannot handle directories with spaces in the path to the tags file, because it puts the directory name in the database and uses spaces to delimit records in the first line (the filename, and also dash-prefixed options cscope had when constructing the database, so there's no way to know for sure where the filename ends). Vim cannot handle spaces either. The limitation does not exist for gtags, but vim's ":cs add" (src/if_cscope.c::cs_add()), typically invoked from gutentags_plus, does not handle spaces correctly itself. It uses spaces in the arguments to break apart into significant subwords, to distinguish other arguments to ':cs'. Lastly the cscope command used by vim in src/if_cscope.c cs_create_connection() does not properly quote the filename when it passes to /bin/sh -c "exec cscope -dl -f %s" so that wouldn't handle spaces either. For now we can live with the limitation that we can't ever use cscope functionality in paths with spaces in them.
The shell default is for clobber to be set, so "echo $$ > $LOCKFILE" will succeed no matter how many times it's run. In other words, this wasn't acting as a lockfile at all, unless the user had set 'noclobber' in their shell's non-interactive profile. In each of these scripts, a trap exists that does cleanup, so the lock files should get removed at script exit no matter what. If the computer crashed or lost power in between though, the stale lock file would be left and have to be manually removed. This is not unlike any other stale lock file issue. However we could check if the pid exists, since it's written to the file. That sounds invasive and non-portable... maybe later.
If we try to establish the lock first before EXIT trap is established, we can exit without removing the lock file of the previous instance. Otherwise we'll correctly exit, but then remove the temp file out from under the already-running instance. Next collision avoidance test will pass and we'll clobber previous instance.
If we exit early before finish, sometimes partial files are left around. Unfortunately these can cause gtags to segfault. "incremental" in gtags should not be thought of as "start where we left off when the last job was prematurely killed" but rather "incrementally update the [previously completely-generated] database by scanning only files that have changed." If the db was not fully generated, we must remove all the partial files from within the trap. This might cause a lot of work to be redone in large source bases, but gtags demonstrably segfaults on partials in some cases, and refuses to continue in other cases, claiming that the db is corrupt. It needs to finish, or its results need to be discarded, but not anything in-between. All that is to say, we will not use 'test -s' and leave a partial around, but just simply remove any files if the handler is entered at all. We must remove the EXIT from the trap-signals list then, because if we leave it, it will always be removed even if fully complete!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
gtags-cscopecurrently has no update script trap, like ctags and cscope have, which does a partial-file cleanup on [premature] exit. if any zero-byte gtags files exist, they will be treated as a prior run by forthcoming gtags, which will be confused that the file is apparently corrupted.The attached add a trap and do cleanup in
gtags, likectags.Also, there was no locking in
gtags, so this patch adds naive locking, same as the other scripts (iectags) have already.Locking was also fixed in all the update scripts. There was a bug that existence of a lockfile was ignored. The noclobber option needs to be set in the shell, in order for 'set -e' to force an exit if the file exists already on redirect. There is a patch included to fix this. While it should be fixed, it may result in more user reports of apparent update script failure, since locking did not work before, and simultaneous attempt was never noticed.
It might be better to ignore some sources of error and exit 0 anyways so the simultaneous access is not a reported issue. But if we did this, it could hide a previous aborted attempt leaving files, and then never be able to start again. Some warning might be in order.
The changes should work both with and without
g:gutentags_cache_dir. In this patch, the lock files are kept alongside the tags files. The other option would have been to put them in the scanned directory.Fixes #319