From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8631 invoked by alias); 6 Sep 2014 18:52:06 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 33116 Received: (qmail 17173 invoked from network); 6 Sep 2014 18:52:04 -0000 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 From: Bart Schaefer Message-id: <140906115126.ZM20019@torch.brasslantern.com> Date: Sat, 06 Sep 2014 11:51:26 -0700 In-reply-to: Comments: In reply to Mikael Magnusson "Re: zsh got stuck without any message because of history lock file" (Sep 6, 11:03am) References: <53594068.4040503@googlemail.com> <140424100228.ZM10689@torch.brasslantern.com> <140424220232.ZM11424@torch.brasslantern.com> X-Mailer: OpenZMail Classic (0.9.2 24April2005) To: zsh workers Subject: Re: zsh got stuck without any message because of history lock file MIME-version: 1.0 Content-type: text/plain; charset=us-ascii On Sep 6, 11:03am, Mikael Magnusson wrote: } } > I'm not 100% sure it's because of this change, but recently, I've had } > new terminals stuck forever on startup until I exit all open terminals } > logged in as that user (or possibly just a specific one of them). It } > happens very spuriously, maybe a few times per week. I have } > histfcntllock set. On one occasion I tried moving the .history file } > instead of exiting, and this also immediately unstuck all new } > terminals. } } I'm still seeing reports of this every now and then in #zsh (and I } have the option unset so I'm not sure if it happens to me anymore.) If } nobody fixed this, it's probably still a problem. So, a few things. The unlockhistfile() operation assumes that closing the flock_fd is enough to release the lock. I can't quote chapter and verse for this from e.g. the POSIX spec, but I believe that to be correct. However, lockhistfile() does not call flockhistfile() when it has already been locked (flock_fd >= 0), and in that case it falls through to file-based locking again. This isn't a problem except when hend() is deciding whether to call savehistfile(); it only considers whether the file is already locked in the case of SHARE_HISTORY, but may call savehistfile() for both SHARE_HISTORY and INC_APPEND_HISTORY et al., and savehistfile() calls lockhistfile() again. Additionally, lockhistfile() only increments the lockhistct counter when file locking, not when fcntl locking, which leads to an incorrect answer from histfileIsLocked(). So I think there's a potential problem where INC_APPEND_HISTORY could apply both locking mechanisms at different times, leading to a race. These were all assumptions about both kinds of locking being applied that I missed before. The "better performance" from HIST_FCNTL_LOCK I guess just mean that it would fail faster on permission problems or lock collisions, not that it would improve performance overall. We can either apply the additional patch below, or back out some of the last one to restore the "always locks both ways" behavior. diff --git a/Src/hist.c b/Src/hist.c index 770d559..d29a65a 100644 --- a/Src/hist.c +++ b/Src/hist.c @@ -2490,6 +2490,9 @@ flockhistfile(char *fn, int keep_trying) struct flock lck; int ctr = keep_trying ? 9 : 0; + if (flock_fd >= 0) + return 0; /* already locked */ + if ((flock_fd = open(unmeta(fn), O_RDWR | O_NOCTTY)) < 0) return errno == ENOENT ? 0 : 2; /* "successfully" locked missing file */ @@ -2768,12 +2771,6 @@ lockhistfile(char *fn, int keep_trying) if (!fn && !(fn = getsparam("HISTFILE"))) return 1; -#ifdef HAVE_FCNTL_H - if (isset(HISTFCNTLLOCK) && flock_fd < 0) { - return flockhistfile(fn, keep_trying); - } -#endif - if (!lockhistct++) { struct stat sb; int fd; @@ -2786,6 +2783,11 @@ lockhistfile(char *fn, int keep_trying) # endif #endif +#ifdef HAVE_FCNTL_H + if (isset(HISTFCNTLLOCK)) + return flockhistfile(fn, keep_trying); +#endif + lockfile = bicat(unmeta(fn), ".LOCK"); /* NOTE: only use symlink locking on a link()-having host in order to * avoid a change from open()-based locking to symlink()-based. */