git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>,
	Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: [PATCH 3/4] trace: lock the trace file to avoid racy trace_write() calls
Date: Thu, 09 Aug 2018 10:35:28 -0700 (PDT)	[thread overview]
Message-ID: <a53e72198ad690a968c12d22e9f2639130a36d78.1533836122.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.17.git.gitgitgadget@gmail.com>

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When multiple processes try to write to the same file, it is not
guaranteed that everything works as expected: those writes can overlap,
and in the worst case even lose messages.

This happens in t/t5552-skipping-fetch-negotiator.sh, where we abuse the
`GIT_TRACE` facility to write traces of two concurrent processes (`git
fetch` and `git upload-pack`) to the same file, and then verify that the
trace contains certain expected breadcrumbs.

To remedy this, let's lock the file descriptors for exclusive writing,
using the function we just introduced in the previous commit.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 trace.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/trace.c b/trace.c
index fc623e91f..6f97dde27 100644
--- a/trace.c
+++ b/trace.c
@@ -114,11 +114,20 @@ static int prepare_trace_line(const char *file, int line,
 
 static void trace_write(struct trace_key *key, const void *buf, unsigned len)
 {
-	if (write_in_full(get_trace_fd(key), buf, len) < 0) {
+	int fd = get_trace_fd(key), locked;
+
+	locked = !lock_or_unlock_fd_for_appending(fd, 1);
+	if (!locked && errno != EBADF)
+		warning("unable to lock file descriptor for %s: %s",
+			key->key, strerror(errno));
+	if (write_in_full(fd, buf, len) < 0) {
 		warning("unable to write trace for %s: %s",
 			key->key, strerror(errno));
 		trace_disable(key);
 	}
+	if (locked && lock_or_unlock_fd_for_appending(fd, 0) < 0)
+		warning("failed to remove lock on fd for %s: %s",
+			key->key, strerror(errno));
 }
 
 void trace_verbatim(struct trace_key *key, const void *buf, unsigned len)
-- 
gitgitgadget


  parent reply	other threads:[~2018-08-09 17:35 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-09 17:35 [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE Johannes Schindelin via GitGitGadget
2018-08-09 17:35 ` [PATCH 1/4] Introduce a function to lock/unlock file descriptors when appending Johannes Schindelin via GitGitGadget
2018-08-09 19:01   ` Junio C Hamano
2018-08-10 18:32     ` Johannes Schindelin
2018-08-09 17:35 ` [PATCH 2/4] mingw: implement lock_or_unlock_fd_for_appending() Johannes Schindelin via GitGitGadget
2018-08-09 17:35 ` Johannes Schindelin via GitGitGadget [this message]
2018-08-09 17:35 ` [PATCH 4/4] trace: verify that locking works Johannes Schindelin via GitGitGadget
2018-08-09 19:47 ` [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE Jeff King
2018-08-09 20:49   ` Junio C Hamano
2018-08-09 21:08     ` Junio C Hamano
2018-08-09 21:32       ` Jeff King
2018-08-10 14:09     ` Jeff King
2018-08-10 15:58       ` Junio C Hamano
2018-08-10 15:58       ` Junio C Hamano
2018-08-10 16:43       ` Johannes Schindelin
2018-08-10 17:15         ` Jeff King
2018-08-10 18:40           ` Junio C Hamano
2018-08-10 19:34           ` Johannes Schindelin
2018-08-10 16:15 ` Johannes Sixt
2018-08-10 16:51   ` Jeff Hostetler
2018-08-10 16:57     ` Jeff Hostetler
2018-08-10 17:08     ` Johannes Sixt
2018-08-10 18:34   ` Junio C Hamano
2018-08-10 18:56     ` Jeff King
2018-08-13 19:02     ` [PATCH] mingw: enable atomic O_APPEND Johannes Sixt
2018-08-13 20:20       ` Junio C Hamano
2018-08-13 21:05         ` Johannes Sixt
2018-08-13 21:22           ` Ævar Arnfjörð Bjarmason
2018-08-13 21:55             ` Junio C Hamano
2018-08-13 22:37             ` Jeff King
2018-08-14 13:47               ` Ævar Arnfjörð Bjarmason
2018-08-14 14:53                 ` Jeff King
2018-08-14 18:29               ` Johannes Sixt
2018-08-14 19:17                 ` Jeff King
2018-08-14 13:01       ` Jeff Hostetler
2018-08-14 14:38         ` Junio C Hamano
2018-08-10 19:47 ` [PATCH v2 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE Johannes Schindelin via GitGitGadget
2018-08-10 19:47   ` [PATCH v2 1/4] Introduce a function to lock/unlock file descriptors when appending Johannes Schindelin via GitGitGadget
2018-08-10 20:05     ` Junio C Hamano
2018-08-10 21:31       ` Johannes Schindelin
2018-08-10 19:47   ` [PATCH v2 2/4] mingw: implement lock_or_unlock_fd_for_appending() Johannes Schindelin via GitGitGadget
2018-08-10 19:47   ` [PATCH v2 3/4] trace: lock the trace file to avoid racy trace_write() calls Johannes Schindelin via GitGitGadget
2018-08-10 19:47   ` [PATCH v2 4/4] trace: verify that locking works Johannes Schindelin via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a53e72198ad690a968c12d22e9f2639130a36d78.1533836122.git.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).