Opened 11 years ago

Closed 11 years ago

#1705 defect closed fixed (fixed)

Test failure on win32 (FilePathTestCase.testGetAndSet)

Reported by: teratorn Owned by: teratorn
Priority: normal Milestone:
Component: core Keywords: win32
Cc: Glyph, Jean-Paul Calderone Branch:


twisted.test.test_paths.FilePathTestCase.testGetAndSet fails on win32 due to the inconsistent behavior of os.rename across platforms, and is currently marked TODO.

On UNIX os.rename will overwrite the destination in most cases. On win32 it will never overwrite.

FilePath's setContent method relies on overwriting the path with os.rename if it already exists.

Why, exactly, does setContent create a tmp file and move it over the destination instead of just writing the content directly to the destination?

Change History (13)

comment:1 Changed 11 years ago by teratorn

Status: newassigned

comment:2 Changed 11 years ago by teratorn

Cc: Glyph Moshe Zadka added

I guess the reason is that os.rename is atomic. If anything bad happens during the setContent() call, you know that the original file is still intact.

So, do we really care about that behavior? If so, I can look in to how to implement it on Windows. Otherwise, we can delete the original file before the rename, if it exists. Or just open the destination and write to it directly.

Glyph, Moshez, I'm CC'ing you because svn blame says you are the likely people to bug :) Comments?

comment:3 Changed 11 years ago by Glyph

You've correctly identified the reason why, and yes, we care about that behavior. It is particularly handy on UNIX if, for example, you have write permission to the directory but not the file in question.

It may be impossible to provide this particular guarantee on Windows though, without some kind of atomic rename. In that case I'm not sure what the right thing to do is.

comment:4 Changed 11 years ago by teratorn

Well, I've Googled, I've scoured the PSDK, and I've searched the MSDN all pretty hard. Found quite a few people who claim an atomic rename isn't possible on win32, and nothing to the contrary :(

We could check if the platform is win32, and only then write the data directly to the destination?

comment:5 Changed 11 years ago by teratorn

One possibility is MoveFileEx() which is available on NT4 and above. It allows a flag that will cause the destination to be overwritten if it exists.

I'm still not sure if it's atomic though. It doesn't say one way or the other in the official documentation, though I've seen a few mailing list posts that claim it is. Who knows?

comment:6 Changed 11 years ago by teratorn

Keywords: review added
Owner: changed from teratorn to Glyph
Status: assignednew

After further investigation, I'm satisfied that an atomic rename() on win32 isn't possible using a any standard APIs.

Branch is ready for review. Current implementation writes content directly to the destination file if the platform is Windows.

Glyph, I'm assigning to you. Please reassign or let me know if you want someone else to look at it.

comment:7 Changed 11 years ago by Glyph

Status: newassigned

I will review this later tonight. Thanks for investigating.

comment:8 Changed 11 years ago by Jean-Paul Calderone

I think setContent should still write to a separate file on Windows. It can delete the original and then safely rename the new file. This is better than writing directly to the new file since with a little more application-level code it is at least possible to recover from a mid-setContent failure using the separate-file scheme, but completely impossible using the direct-write scheme.

comment:9 Changed 11 years ago by Glyph

Keywords: review removed
Owner: changed from Glyph to teratorn
Status: assignednew

I agree with what JP said; it should still do write-aside. As the size of the value value provided to setContent increases, the probability of dying in mid-write increases whereas the probability of dying between the delete and the rename remains constant. (Or, you know, constant to within one unit of "NTFS sucks ass")

comment:10 Changed 11 years ago by teratorn

Keywords: review added
Owner: changed from teratorn to Glyph

Yep makes sense. Changes committed. OK to merge?

comment:11 Changed 11 years ago by teratorn

Cc: Jean-Paul Calderone added; Moshe Zadka removed

comment:12 Changed 11 years ago by Glyph

Keywords: review removed
Owner: changed from Glyph to teratorn

Yeah. Merge away.

comment:13 Changed 11 years ago by teratorn

Resolution: fixed
Status: newclosed

(In [16818]) Merge testGetAndSet-win32-1705

Author: teratorn Review: glyph Fixes #1705

On win32, make FilePath.setContent() remove the destination file first because os.rename() will not overwrite an existing file. There doesn't appear to be any way to do atomic renames on win32 so this is as safe as we can make it.

Note: See TracTickets for help on using tickets.