Opened 6 years ago

Closed 4 years ago

#3454 defect closed duplicate (duplicate)

FilePath operations which are atomic on POSIX should be atomic on Windows

Reported by: glyph Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: teratorn, spiv, exarkun Branch:
Author: Launchpad Bug:

Description

os.rename doesn't overwrite on Windows, and so in FilePath.setContent we do a crummy platform check and delete the file in advance of moving the new one over it.

However, ReplaceFile and MoveFileEx with the MOVEFILE_REPLACE_EXISTING flag do overwrite, and we could use one of those in the implementation of moveTo instead.

Change History (9)

comment:1 Changed 6 years ago by exarkun

Snag: there are currently no test coverage for the atomicity of any operations on any platforms. It would be simpler to provide consistency by guaranteeing nothing. :)

comment:2 Changed 6 years ago by teratorn

  • Cc teratorn added

I'm pretty sure than an atomic rename is impossible on win32 without using the transactional filesystem APIs available in Vista and Server 2008.

Some time ago I researched this question very deeply. See #1705

Using a move-and-replace API doesn't really buy us anything over using the current delete-then-rename scheme. It can still fail and leave the filesystem in an inconsistent state.

Providing atomic semantics on Vista and Server 2008 would be *nice*, I guess, but perhaps impractical. Is there any way to call MoveFileTransacted without introducing dependencies? On Python 2.3 and up?

comment:3 Changed 6 years ago by spiv

I think teratorn is right: this isn't feasible.

AFAIK, these APIs can all fail if the destination file to be overwritten is in use. That's fundamentally unlike POSIX, and that part of POSIX is a big part of what's so useful about atomic rename.

If you find a way to do this, I'd love to know about it; it'd be useful for bzr... we'd be able to get rid of bzrlib.osutils.fancy_rename. But I don't believe it is possible.

comment:4 Changed 6 years ago by spiv

  • Cc spiv added

comment:5 follow-up: Changed 6 years ago by exarkun

  • Cc exarkun added

We can use pywin32 to call pretty much any Windows APIs we like. Twisted on Windows requires pywin32 for most things already, so it's not even a new dependency. ctypes is another possibility.

Of course, it still may be impossible, but I know that there's room for improvement from the current situation.

comment:6 in reply to: ↑ 5 Changed 6 years ago by teratorn

Replying to exarkun:

We can use pywin32 to call pretty much any Windows APIs we like. Twisted on Windows requires pywin32 for most things already, so it's not even a new dependency.

Yep, we can use it to call MoveFileEx, but it doesn't have a binding for MoveFileTransacted.

ctypes is another possibility.

Only problem is that it's only available in the py 2.5 stdlib and up.

Of course, it still may be impossible, but I know that there's room for improvement from the current situation.

For fun and future reference:
http://stackoverflow.com/questions/167414/is-an-atomic-file-rename-with-overwrite-possible-on-windows

I'm convinced that it's impossible to have a real atomic operation without MoveFileTransacted.. I think the only reasonable thing to do is to assume that this is the case until proven otherwise.

So, should we use MoveFileEx with MOVEFILE_REPLACE_EXISTING as an improvement over separate delete and rename calls? If so, should this be a hard dependency on pywin32 or should it fall back to delete/rename silently?

My vote is for doing this, and making it a hard dependency. I see pywin32 as a standard library that just wasn't included... and as you say it's already depended on for core functionality (spawning processes, etc). Also, I've often wanted to use pywin32 for things, by shyed away from it because it wasn't an official dependency.

Should we check for ctypes and Vista/2008, and in that case use a transactional move? If so, how can we do the platform check? Or perhaps ctypes can just tell us if the function exists.

This would be a pretty simple block of code, with no maintenance requirements, assuming we don't screw up the platform check. And it would provide a valuable improvement for folks targetting up-and-coming Windows platforms. Any objections?

comment:7 follow-up: Changed 5 years ago by khorn

This appears to be a duplicate of #3004

comment:8 in reply to: ↑ 7 Changed 4 years ago by glyph

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

Replying to khorn:

This appears to be a duplicate of #3004

At first I thought maybe not because this is about everything and that's just about setContent, but really whatever addresses this is going to need to deal with multiple methods. So yeah, let's just have one ticket.

comment:9 Changed 3 years ago by <automation>

  • Owner glyph deleted
Note: See TracTickets for help on using tickets.