https://pantsbuild.org/ logo
Powered by
#general
  • a

    ancient-stone-50795

    03/23/2016, 10:58 AM
    I've updated the issue and I've got a proposed fix up at https://rbcommons.com/s/twitter/r/3598/. TL;DR I thought about using the fasteners library but settled on our existing wrapper to lockfile: OwnerPrintingPIDLockFile
  • b

    bored-art-40741

    03/23/2016, 1:04 PM
    Ugh, it doesn't use flock?
  • b

    bored-art-40741

    03/23/2016, 1:05 PM
    @ancient-stone-50795: I wouldn't support the solution you're proposing
  • b

    bored-art-40741

    03/23/2016, 1:05 PM
    That suffers from the same issue that our nailgun pid locking used to: a new process can roll in and have the same pid
  • b

    bored-art-40741

    03/23/2016, 1:05 PM
    We should just use flock directly, it's not that hard
  • a

    ancient-stone-50795

    03/23/2016, 1:20 PM
    We need to do something quickly: I have found more anecdotal reports of this happening in the past two weels. The solution I propose is what we use today for the global pants lock and the pants daemon, and we don't run into issues with it, so I would argue it is a step forward. I looked into replacing them all with the fastener's library, but maintaining the semantics of the existing OwnerPidLockfile isn't a 1:1 mapping to the fasteners library.
  • a

    ancient-stone-50795

    03/23/2016, 1:22 PM
    Even if it does have the bug you propose, it will print out the name of the command that is holding the lock and you'll see what the problem is. Today, the behavior is that pants hangs forever with no indication of what is wrong.
  • a

    ancient-stone-50795

    03/23/2016, 1:24 PM
    @bored-art-40741: Wouldn't you agree that even if we band-aid with this fix I propose that it makes it easier to fix the problem you point out everywhere the next time someone? Besides this, there are two other bugfixes we found in our upgrade to 0.0.77 and we just want to get our fixes in place and get out a stable release.
  • a

    ancient-stone-50795

    03/23/2016, 1:27 PM
    If that isn't acceptable, I'll propose reverting the ivy lock change.
  • b

    bored-art-40741

    03/23/2016, 1:28 PM
    Could just add a no-lock option to the subsystem
  • b

    bored-art-40741

    03/23/2016, 1:29 PM
    I think if we're bothering with band-aid and release, we'd might as well just use a proper flock. I'll write it up when I get into the office
  • a

    ancient-stone-50795

    03/23/2016, 1:29 PM
    No, I'll add a --lock option and leave it false by default because this implementation is too broken to use in production.
  • b

    bored-art-40741

    03/23/2016, 1:30 PM
    Then just revert.
  • a

    ancient-stone-50795

    03/23/2016, 1:31 PM
    ok
  • b

    bored-art-40741

    03/23/2016, 1:32 PM
    Not that that's anywhere close to safe, but it's at least not a regression
  • a

    ancient-stone-50795

    03/23/2016, 1:32 PM
    It is completely safe in our environment - we are protected by the global pants lock. You won't be able to just use flock to fix the other implementations OwnerPidLockfile. it allows recursive locking within the same process.
  • a

    ancient-stone-50795

    03/23/2016, 1:32 PM
    You'll have to keep a stack of internal locks and unwind them.
  • b

    bored-art-40741

    03/23/2016, 1:33 PM
    This lock shouldn't be reentrant.
  • a

    ancient-stone-50795

    03/23/2016, 1:34 PM
    No, but the problem you are all up in arms about exists in 2 other places. If you are going to stand your ground that this is a vital problem, we should fix it everywhere.
  • a

    ancient-stone-50795

    03/23/2016, 1:34 PM
    And that lock is designed to be re-entrant
  • a

    ancient-stone-50795

    03/23/2016, 1:35 PM
    And if you do that, you can just apply the patch I'm proposing to make the ivy lock inherit the same fix
  • b

    bored-art-40741

    03/23/2016, 1:36 PM
    It's an insufficient fix. I don't see the RLock reasoning: this is not supposed to be a reentrant lock. In any of the places it's used. (By "this" I mean the per directory lock around ivy resolution)
  • b

    bored-art-40741

    03/23/2016, 1:36 PM
    I'm on a phone. Revert if you want; I'll take a closer look in the office
  • a

    ancient-stone-50795

    03/23/2016, 1:41 PM
    When you get into the office, read through the github issue and consider the best way fix it everywhere. I don't need a release to fix this, but it is broken for all pants users > 0.0.75 simply by hitting CTRL-C at an inopportune time. I don't want to leave it the way it is for the next release.
  • a

    ancient-stone-50795

    03/23/2016, 2:00 PM
    I updated https://rbcommons.com/s/twitter/r/3598/ I don't need to be in a hurry to revert, but I don't have lots of time today to spend working on an alternative.
  • a

    ancient-stone-50795

    03/23/2016, 2:11 PM
    And the reason why 
    lockfile
    doesn't use 
    flock()
    is explained in the https://pypi.python.org/pypi/lockfile documentation. Its for portability between *NIX and windows implementations. There are other locking schemes you can use, but it isn't clear that any of them share or solve the stale lockfile issue.
  • b

    bored-art-40741

    03/23/2016, 2:20 PM
    Just got in
  • b

    bored-art-40741

    03/23/2016, 2:21 PM
    We don't pretend to support windows, so that shouldn't preclude us from using flock ourselves
  • a

    ancient-stone-50795

    03/23/2016, 2:21 PM
    I agree
  • b

    bored-art-40741

    03/23/2016, 2:22 PM
    Okay yeah, I messed up when using lockfile. I misread
    Copy code
    Unlike the Windows msvcrt.locking() function, the Unix fcntl.flock(), fcntl.lockf() and the deprecated posixfile module, the API is identical across both Unix (including Linux and Mac) and Windows platforms. The lock mechanism relies on the atomic nature of the link() (on Unix)...
    to mean "still has the properties of flock"
1...108310841085...2420Latest