Opened 7 years ago

Last modified 3 years ago

#1061 new defect

Buildbot locks downloaded files

Reported by: parshin Owned by:
Priority: major Milestone: 0.8.x
Version: 0.8.2 Keywords: windows, transfer
Cc:

Description (last modified by sa2ajj)

OS: Windows XP SP2.

Steps to reproduce bug:

  1. Create two builds both with the following build steps:
    1. Download file from the master (FileDownload)
    2. Run shell command, which uses this file
  2. Run both builds simultaneously at the same slave.

In my case downloaded file was an executable, shell command was batch file which simply starts the executable.

With rather high probability (~50%) one of builds fails because of the executable was locked by another process.


Looks like the reason is the following. When files are downloaded at slave, slave process has two open handles. Next, if one build has finished download first, it starts separate process for shell command, which inherits all handles of the parent process (slave) including open handle for other downloading file (it is default behavior of Twisted spawnProcess() ). As a result, downloaded file is locked even after second build finishes download and close its handle.


I've tried to fix this problem by preventing handle inheritance by setting HANDLE_FLAG_INHERIT flag of handle to zero. Attached patch fixes that bug for me. Maybe, better solution is to download to temporary file and rename it after download.

Attachments (1)

handleinherit.2.patch (745 bytes) - added by parshin 7 years ago.

Download all attachments as: .zip

Change History (15)

Changed 7 years ago by parshin

comment:1 Changed 7 years ago by ayust

  • Milestone changed from undecided to 0.8.5

comment:2 Changed 6 years ago by dustin

Hm, spawnProcess is frustrating! The same probably happens on POSIX, but of course, on POSIX holding a file open does not "lock" it. I think your solution is a good one, but it needs to be written in such a way that it will work on POSIX systems, too - it can't be merged as-is. Can you update it to do so?

comment:3 Changed 6 years ago by dustin

  • Keywords transfer added

comment:4 Changed 6 years ago by dustin

Wouldn't this need to happen for *every* open file?

Does this still occur with the latest version of Twisted?

comment:5 Changed 6 years ago by dustin

  • Milestone changed from 0.8.5 to 0.8.+

I'm not sure this can be easily fixed in 0.8.5.

I suspect that all process spawning in Buildbot should be farmed out to a helper process. I wonder how difficult it would be to use Multiprocessing to accomplish that?

comment:6 Changed 6 years ago by parshin

I can reproduce this bug with buildbot 0.8.4p2 and twisted 11.0.0 on Windows. But can't reproduce it on Ubuntu 11.04, buildbot 0.8.5rc1, twisted 11.0.0 ...

So, maybe it is windows-specific problem (twisted.spawnProcess() uses platform-specific code)...

comment:7 Changed 6 years ago by dustin

Well, it's NTFS-specific, since NTFS does not allow the same file to be opened more than once.

comment:8 Changed 3 years ago by dustin

  • Milestone changed from 0.8.+ to 0.9.+

Ticket retargeted after milestone closed

comment:9 Changed 3 years ago by Jason

FTR We've applied this patch locally and it seems to fix the problem we were experiencing on Windows.

Jason

comment:10 Changed 3 years ago by sa2ajj

  • Description modified (diff)

Jason, what version of Buildbot did you patch?

comment:11 Changed 3 years ago by Jason

v0.8.8-651-g150168a

Plus some other misc patches to the master server.

comment:12 Changed 3 years ago by Jason

Actually on Windows it appears we are running 0.8.8.

comment:13 Changed 3 years ago by sa2ajj

  • Milestone changed from 0.9.+ to 0.8.x

Thank you!

I'll see how this can be adopted to 0.8.10.

It might fix some other problems with transfers which seem to be Windows specific.

Last edited 3 years ago by sa2ajj (previous) (diff)

comment:14 Changed 3 years ago by dustin

The patch leaves a race condition, unfortunately, between the open and the inherit setting. It also doesn't address any other files that might be open within the process for various other reasons.

That said, it's an improvement, so as long as it doesn't interfere with the transfer steps' operation, I'm happy with it.

Note: See TracTickets for help on using tickets.