code review 4968079: os: Remove file lock on Windows

os: Remove file lock on Windows

This lock has been introduced as a fix to emulate Linux' Pread semantic on Windows (see http://codereview.appspot.com/4415041/).
On Linux Pread and Pwrite do not change the current file position but on Windows do.
To emulate Linux semantic, Pread on Windows was implemented as the sequence of Lock, GetFilePos, Read, Seek, Unlock.

This patch solves the problem in a lightweight way, storing a fake file pointer inside File structure.
ReadAt() and WriteAt() can be called from different threads in parallel.

Also, with this patch, all the file reads and writes became overlapped, making it easy to wait for their completion in a non-blocking way (as it is done with sockets).
The same IOCP loop can be shared for sockets and files.

[jp, 2011-09-08 19:03:16]

Hello golang-dev@googlegroups.com (cc: alex.brainman@gmail.com,
golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2011-09-08 19:07:21]

Seems unlikely to play nicely with the rest of
the operating system.

Suppose you build this program as 8.out:

package main
import "fmt"
func main() { fmt.Println(os.Args) }

and then at the command prompt you run

8.out test1 >file
8.out test2 >>file

I bet you will not get two different lines in file.

You could paper over that by sniffing the file
offset at the beginning of the program, but it still
fails if you pass files to other programs using
StartProcess, for example.

I think the lock is a better solution here.
What's the problem with it?

Russ

[jp, 2011-09-08 19:30:24]

On 2011/09/08 19:07:21, rsc wrote:

> You could paper over that by sniffing the file
> offset at the beginning of the program, but it still
> fails if you pass files to other programs using
> StartProcess, for example.

Is it practical to pass the file handles (not pipe handles - they are not
seekable) with a presetup position ?
Never saw something like this.

> I think the lock is a better solution here.

I also tried to make a separate handle for pread/write, but both handles share
the same pointer. Are you sure that the handle passed to the child process will
have its own pointer there? Otherwise even the lock does not guarantee "that
ReadAt shouldn't move the seek offset".

> What's the problem with it?

1. One opinion:
http://groups.google.com/group/golang-dev/browse_thread/thread/92ebe2285855d5...
2. Alex opinion that the lock is too heavy for this issue:
http://codereview.appspot.com/4415041/
3. I am not happy with an OS thread to each pipe blocking call and would rather
have asynchronous file operations.

[brainman, 2011-09-09 02:16:43]

On 2011/09/08 19:30:24, jp wrote:
> 
> Is it practical to pass the file handles (not pipe handles - they are not
> seekable) with a presetup position ?
> Never saw something like this.
> 

You could share file handle with other parts of your program - non-go dll that
you are calling. You could even pass file handles to a different process with
DuplicateHandle.

> ... Are you sure that the handle passed to the child process will
> have its own pointer there? ...
> 

From DuplicateHandle reference:

>>>
The duplicate handle refers to the same object as the original handle.
Therefore, any changes to the object are reflected through both handles. For
example, if you duplicate a file handle, the current file position is always the
same for both handles. For file handles to have different file positions, use
the CreateFile function to create file handles that share access to the same
file.
<<<

> > What's the problem with it?
> 
> 1. One opinion:
>
http://groups.google.com/group/golang-dev/browse_thread/thread/92ebe2285855d5...

Not sure what problem you are referring here.

> 2. Alex opinion that the lock is too heavy for this issue:
> http://codereview.appspot.com/4415041/

But I don't think your proposal any simpler. If you end up calling
GetOverlappedResult, after Read/Write, you solution would be even less efficient
too.

> 3. I am not happy with an OS thread to each pipe blocking call and would
rather
> have asynchronous file operations.

I am not worried about that. Yet. If your program does blocking reads from so
many files at the same time, that you are worried about all these wasted
threads, then you shouldn't be using os.File. I think, simplicity of go library
source code is very important. I hate to sacrifice that, unless I have a very
good reason.

Alex

[jp, 2011-09-09 03:08:09]

On 2011/09/09 02:16:43, brainman wrote:
> On 2011/09/08 19:30:24, jp wrote:
> > 
> > Is it practical to pass the file handles (not pipe handles - they are not
> > seekable) with a presetup position ?
> > Never saw something like this.
> > 
> 
> You could share file handle with other parts of your program - non-go dll that
> you are calling. You could even pass file handles to a different process with
> DuplicateHandle.
> 
> > ... Are you sure that the handle passed to the child process will
> > have its own pointer there? ...
> > 
> 
> From DuplicateHandle reference:
> 
> >>>
> The duplicate handle refers to the same object as the original handle.
> Therefore, any changes to the object are reflected through both handles. For
> example, if you duplicate a file handle, the current file position is always
the
> same for both handles. For file handles to have different file positions, use
> the CreateFile function to create file handles that share access to the same
> file.

That means you need an interprocess lock in os.File to preserve the file pointer
during pread. Because pread of the same file object can be done not only from
different threads but also from different processes.
Looks like you blame me that I introduce a bug which is not fixed nor reported
in the present version.

> <<<
> 
> > > What's the problem with it?
> > 
> > 1. One opinion:
> >
>
http://groups.google.com/group/golang-dev/browse_thread/thread/92ebe2285855d5...
> 
> Not sure what problem you are referring here.
> 
> > 2. Alex opinion that the lock is too heavy for this issue:
> > http://codereview.appspot.com/4415041/
> 
> But I don't think your proposal any simpler. If you end up calling
> GetOverlappedResult, after Read/Write, you solution would be even less
efficient
> too.

For fast reads (all the short reads used in the Go test suite) ReadFile returns
with no ERROR_IO_PENDING.

GetOverlappedResult to be removed and long reads are to be waited in IOCP, as it
is done for sockets in the net package.
Anyway, it results in an extra API call, you are right.

But if ReadFile would block for a while, it results in an extra thread (more
than one API call and a few context switches).


> 
> > 3. I am not happy with an OS thread to each pipe blocking call and would
> rather
> > have asynchronous file operations.
> 
> I am not worried about that. Yet. If your program does blocking reads from so
> many files at the same time, that you are worried about all these wasted
> threads, then you shouldn't be using os.File. I think, simplicity of go
library
> source code is very important. I hate to sacrifice that, unless I have a very
> good reason.

I think, using the same IOCP for sockets, files (and probably timers as well) is
an elegant and simple solution.

[brainman, 2011-09-09 03:30:12]

On 2011/09/09 03:08:09, jp wrote:
> 
> Looks like you blame me that I introduce a bug which is not fixed nor reported
> in the present version.

I certainly not blame you for anything. I wrote the code, so I can't blame
anyone but myself. If you feel that way, I apologize.

> 
> I think, using the same IOCP for sockets, files (and probably timers as well)
is
> an elegant and simple solution.

We could agree or disagree here. But let's not argue about this, but lets
understand what are we fixing here.

I agree, the existing solution is far from perfect, but I don't think your
proposal is any better. For example, existing Read and Write functions are safe
to use even across different processes.

Alex

[jp, 2011-09-09 11:57:43]

On 2011/09/09 03:30:12, brainman wrote:
> For example, existing Read and Write functions are safe to use even across
different processes.

Unless no one of the processes calls ReadAt changing the global file pointer for
a while.
Unless you do not use the same (or duplicated) handle in another os.File, even
in the same process (there will be 2 independed locks protecting a single file
pointer).

The case with the duplicated handles and the handles shared between processes is
buggy in both versions.
Well, we can leave it aside, and can keep the current logic with lock.
It does not hamper to move towards the async file IO (in order to reduce number
of threads which are busy with nothing but IO waiting).

Any objection on CL witch will keep the os.File locks as they made in the
present version but will use FILE_FLAG_OVERLAPPED as a default open mode and
delegate the incomplete IO operations (ReadFile/WriteFile returned
ERROR_IO_PENDING) to the IOCP loop in the net module (or moved from the net
module to a location like runtime/iocp) turning the blocking OS calls into
waiting for channels?

[jp, 2011-09-09 12:14:37]

On 2011/09/09 02:16:43, brainman wrote:
> > 1. One opinion:
> >
>
http://groups.google.com/group/golang-dev/browse_thread/thread/92ebe2285855d5...
> 
> Not sure what problem you are referring here.
> 

My proposal solves it with "Option 2 would be to modify the os package to open
all files with FILE_FLAG_OVERLAPPED.  Not sure what other implications that
change might have."

An about the lock again...
The lock does not allow to Write to the os.File during the time when os.File is
blocked in Read (and vice versa: you cannot Read until Write is completed).
Practical case with slow files (pipes, serial ports, etc)..

[brainman, 2011-09-11 08:30:46]

On 2011/09/09 11:57:43, jp wrote:
> 
> It does not hamper to move towards the async file IO (in order to reduce
number
> of threads which are busy with nothing but IO waiting).

Not interested. Unless you could provide some real case scenario where this
would be beneficial.

> Any objection on CL witch will ...

Yes. I would like to keep current os.File implementation simple.

Alex

[brainman, 2011-09-11 08:32:36]

On 2011/09/09 12:14:37, jp wrote:
> The lock does not allow to Write to the os.File during the time when os.File
is
> blocked in Read (and vice versa: you cannot Read until Write is completed).
> Practical case with slow files (pipes, serial ports, etc)..

If you use serial port or pipe, you should not be using os.File.

Alex

[jp, 2011-09-11 08:57:17]

On 2011/09/11 08:32:36, brainman wrote:
> On 2011/09/09 12:14:37, jp wrote:
> > The lock does not allow to Write to the os.File during the time when os.File
> is
> > blocked in Read (and vice versa: you cannot Read until Write is completed).
> > Practical case with slow files (pipes, serial ports, etc)..
> 
> If you use serial port or pipe, you should not be using os.File.

Why not?
They are definitely OS files, only slower than files on your SSD.
Files on a network mounted partition can suffer from the lock the same way.

[brainman, 2011-09-12 01:03:09]

On 2011/09/11 08:57:17, jp wrote:
> > 
> > If you use serial port or pipe, you should not be using os.File.
> 
> Why not?
> They are definitely OS files, ...

The only thing in common is that you can read and write from all of them. That
is where the difference ends:
1) you can't set communication parameters and timeouts on files;
2) you can't seek on serial port;
3) you can't "listen" on files;

This is what I see when I look at their APIs.

Alex

[jp, 2011-09-12 09:54:19]

On 2011/09/12 01:03:09, brainman wrote:
> On 2011/09/11 08:57:17, jp wrote:
> > > 
> > > If you use serial port or pipe, you should not be using os.File.
> > 
> > Why not?
> > They are definitely OS files, ...
> 
> The only thing in common is that you can read and write from all of them. That
> is where the difference ends:
> 1) you can't set communication parameters and timeouts on files;
files on disk do have other parameters (inode number, content of symlink,
content of ntfs streams, etc). there is the access to os handle to do thinks
like this.
> 2) you can't seek on serial port;
as well as disk files opened with O_APPEND. A log file on a network drive
behaves very similar to a pipe: you cannot seek and writes take long time.
> 3) you can't "listen" on files;
not sure I understood what is "listen" on serial port or pipe
> 
> This is what I see when I look at their APIs.
> 
> Alex

[brainman, 2011-09-12 11:23:02]

On 2011/09/12 09:54:19, jp wrote:
> > 3) you can't "listen" on files;
> not sure I understood what is "listen" on serial port or pipe

ConnectNamedPipe

Alex

[rsc, 2012-06-03 04:31:45]

removing reviewers