Opened 8 years ago

Closed 8 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, exarkun Branch:
Author: Launchpad Bug:

Description

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 8 years ago by teratorn

  • Status changed from new to assigned

comment:2 Changed 8 years ago by teratorn

  • Cc glyph moshez 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 8 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 8 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 8 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 8 years ago by teratorn

  • Keywords review added
  • Owner changed from teratorn to glyph
  • Status changed from assigned to new

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 8 years ago by glyph

  • Status changed from new to assigned

I will review this later tonight. Thanks for investigating.

comment:8 Changed 8 years ago by exarkun

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 8 years ago by glyph

  • Keywords review removed
  • Owner changed from glyph to teratorn
  • Status changed from assigned to new

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 8 years ago by teratorn

  • Keywords review added
  • Owner changed from teratorn to glyph

Yep makes sense. Changes committed. OK to merge?

comment:11 Changed 8 years ago by teratorn

  • Cc exarkun added; moshez removed

comment:12 Changed 8 years ago by glyph

  • Keywords review removed
  • Owner changed from glyph to teratorn

Yeah. Merge away.

comment:13 Changed 8 years ago by teratorn

  • Resolution set to fixed
  • Status changed from new to closed

(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.