Change https://go.dev/cl/472475 mentions this issue: os,net: support zero-copy from file and TCP socket to Unix socket

/cc @rsc @ianlancetaylor

Quite an unfortunate collision of names.
While not automatic, would io.Copy(unixConn1.File(), unixConn2.File()) be zero copy?

Quite an unfortunate collision of names. While not automatic, would io.Copy(unixConn1.File(), unixConn2.File()) be zero copy?

Interesting question, I've never verified whether that approach works but I doubt it because Unix domain sockets don’t actually write the data they send to the disk like regular files, as that would be far too slow. Instead, all the data is retained within kernel memory; the only point of the socket file on disk is maintaining a reference to the sockets and giving it filesystem permissions to control access, thus, copy_file_range(2) might not work for this kind of case.

It seems awkward for something like os.File to have both ReadFrom and WriteTo methods. io.Copy will prefer the WriteTo method, which seems to mean that if the WriteTo method can't optimize it will need to reimplement part of io.Copy. The situation with the two methods going back and forth is already rather confusing. I would prefer to not make it more confusing.

It's possible that something like #41198 can help here.

In fact, I intended to fall back to ReadFrom in the new WriteTo method in order not to change the process of io.Copy, see the CL. But you're suggesting that we don't do the fallback in WriteTo and just return ErrUnsupported and update io.Copy to adapt it? @ianlancetaylor

I think it's worth considering. I don't know for sure that it is better. I do think that it is confusing for WriteTo to call ReadFrom. The current code is already hard to understand and change, and I'm concerned about making that worse.

Any other comments from the team on this proposal? I'd like to solicit more opinions here, thanks!

Change https://go.dev/cl/475535 mentions this issue: os: don't hide all methods in recursive call to io.Copy

After refreshing my memory of #41198, I don't think it necessarily helps. We can't really change ReadFrom to return ErrUnsupported, because historically it has not done so. Changing it do that would break existing callers outside of the standard library.

But we can do something like CL 475535. I think that approach will permit the cases we want here.

After refreshing my memory of #41198, I don't think it necessarily helps. We can't really change ReadFrom to return ErrUnsupported, because historically it has not done so. Changing it do that would break existing callers outside of the standard library.

But we can do something like CL 475535. I think that approach will permit the cases we want here.

Actually, I didn't plan on returning ErrUnsupported from File.ReadFrom, I was talking about the new method File.WriteTo/TCPConn.WriteTo.

But look at it another way, if we can update the io.copyBuffer in a way like this

	if wt, ok := src.(WriterTo); ok {
		if n, err := wt.WriteTo(dst); n > 0 || err == nil || !errors.Is(err, errors.ErrUnsupported) {
			return n, err
		}
	}
	// Similarly, if the writer has a ReadFrom method, use it to do the copy.
	if rt, ok := dst.(ReaderFrom); ok {
		if n, err := rt.ReadFrom(src); n > 0 || err == nil || !errors.Is(err, errors.ErrUnsupported) {
			return n, err
		}
	}

then we will be able to avoid wasting the user-provided buffer via io.CopyBuffer, and also be able to eliminate all genericReadFrom and genericWriteTo, without breaking the compatibility of the current codebase.

Change https://go.dev/cl/475575 mentions this issue: io: optimize copyBuffer to make use of the user-provided buffer for fallbacks

After refreshing my memory of ErrUnsupported, I don't think that change is appropriate. We should only use ErrUnsupported when the case can't be executed at all. That is not true for ReadFrom or WriteTo. Those methods should do the right thing even when they can't implement a special case. So ErrUnsupported was a red herring. My apologies.

After a second thought, I agree that ReadFrom and WriteTo are general implementations to transfer data and we shouldn't refit them into specific cases just for io.Copy.

Well, this is however not good news for this proposal, we still need an appropriate fallback from WriteTo to ReadFrom inside io.Copy, hoping anything better than my current implementation.

Another way to help this proposal is to add a new interface similar to io.ReaderFrom, try it before WriteTo and ReadFrom in io.Copy, and implement it on UnixConn, although the new interface does look like a compensation for the collision of io.ReaderFrom and io.WriterTo of UnixConn.

Perhaps we could support a wrapper around net.UnixConn that implements a ReadFrom method that is compatible with io.Copy.

You mean an exported struct wrapper of UnixConn which is recommended to use when calling io.Copy with the destination of UnixConn?

Something like this:

package net

type UnixConnReaderFrom struct {
	*UnixConn
}

func (c UnixConnReaderFrom) ReadFrom(r io.Reader) (n int64, err error) {
	// do something with zero-copy...
	
	return
}

func NewUnixConnReaderFrom(c Conn) (*UnixConnReaderFrom, error) {
	uc, ok := c.(*UnixConn)
	if !ok {
		return nil, errors.New("must be a *net.UnixConn")
	}
	return &UnixConnReaderFrom{uc}, nil
}

package main

func main() {
	c, _ := net.Dial("unix", "/tmp/sock")
	uc, _ := net.NewUnixConnReaderFrom(c)
	f, _ := os.Create("log")
	io.Copy(uc, f)
}

Yes, something like that. That may be too horrible, though.

You mean an exported struct wrapper of UnixConn which is recommended to use when calling io.Copy with the destination of UnixConn?

Something like this:

package net

type UnixConnReaderFrom struct {
	*UnixConn
}

func (c UnixConnReaderFrom) ReadFrom(r io.Reader) (n int64, err error) {
	// do something with zero-copy...
	
	return
}

func NewUnixConnReaderFrom(c Conn) (*UnixConnReaderFrom, error) {
	uc, ok := c.(*UnixConn)
	if !ok {
		return nil, errors.New("must be a *net.UnixConn")
	}
	return &UnixConnReaderFrom{uc}, nil
}

package main

func main() {
	c, _ := net.Dial("unix", "/tmp/sock")
	uc, _ := net.NewUnixConnReaderFrom(c)
	f, _ := os.Create("log")
	io.Copy(uc, f)
}

Pros and cons:

• pros: we can support all three zero-copy cases: file -> unix, tcp -> unix and unix -> unix
• cons: users have to use this wrapper explicitly with io.Copy, rather than naturally as in other zero-copy cases

Since the 1.22 tree had opened and we didn't come to a final implementation of this proposal at the time, I'd like to pick up where we left off and continue to discuss it.

Any new thoughts from the team?

Kindly ping @rsc @ianlancetaylor

What do you see as the current proposal? Thanks.

What do you see as the current proposal? Thanks.

Well, we've made several attempts at this proposal:

1. The original implementation was to fall back to ReadFrom method in WriteTo, but it seemed to be confusing, quoted from your comment
2. The next implementation tried to use errors. ErrUnsupported but that, as it turned out, was inapposite.
3. The following approach would bring a new wrapper, it would enable us to support all three zero-copy cases: file -> unix, tcp -> unix and unix -> unix, but it might look a bit hideous

Therefore, I'd like to suggest adding a new interface (given that there is already a collision between net.PacketConn and net.ReaderFrom on UnixConn) for trying zero-copy techniques in io.Copy and having UnixConn implement it, that way, we can get what we want in this proposal and make the zero-copy transfer transparent to developers and avoid an explicit wrapper.

How do you like this new idea? @ianlancetaylor

Sorry, I'm not sure what I was thinking earlier. I don't think we want a new exported function net.NewUnixConnReaderFrom.

I'm not yet seeing a reasonable way to do this.

As I said in the previous comment, is it acceptable to add a new interface say io.TransferFrom with method ReadFrom where we do the zero-copy, and make the type assertion with it in io.Copy, just like ReaderFrom with ReadFrom, then have UnixConn implement it?

I don't think adding another special method that io.Copy recognizes is a great idea. The current situation with ReadFrom and WriteTo is already confusing. That said I don't see how just adding a new interface with a ReadFrom method works, so maybe I am missing something. We already check for an interface with a ReadFrom method.

OK, thanks. I don't think adding another special method that io.Copy recognizes is a good approach. It's already too complicated. If we do want to consider that, we should consider it by itself.

I think we have to face the fact that there might not be a good approach for this proposal based on the current problems of net.UnixConn and our previous discussions. Unless someone else has some new ideas about it, we should invite more people from the team to join this discussion?

Inspired by this CL, I just uploaded a new approach in which we can avoid calling ReadFrom in WriteTo, and we also don't have to modify io.Copy now.

What do you think about it? @ianlancetaylor

Change https://go.dev/cl/515595 mentions this issue: net: don't hide all methods in recursive call to io.Copy

I'm going to have to look at it more, but it seems promising. Thanks.

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

Have all remaining concerns about this proposal been addressed?

The API change is to define WriteTo methods on os.File and net.TCPConn.
When these are invoked by io.Copy with a net.UnixConn as the writer,
they will use a call like Linux’s sendfile(2) to make the copy, if possible.
An implementation is in https://go.dev/cl/472475.

Have all remaining concerns about this proposal been addressed?

The API change is to define WriteTo methods on os.File and net.TCPConn.
When these are invoked by io.Copy with a net.UnixConn as the writer,
they will use a call like Linux’s sendfile(2) to make the copy, if possible.
An implementation is in https://go.dev/cl/472475.

I believe there are no remaining concerns about this proposal now.

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The API change is to define WriteTo methods on os.File and net.TCPConn.
When these are invoked by io.Copy with a net.UnixConn as the writer,
they will use a call like Linux’s sendfile(2) to make the copy, if possible.
An implementation is in https://go.dev/cl/472475.

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The API change is to define WriteTo methods on os.File and net.TCPConn.
When these are invoked by io.Copy with a net.UnixConn as the writer,
they will use a call like Linux’s sendfile(2) to make the copy, if possible.
An implementation is in https://go.dev/cl/472475.

Change https://go.dev/cl/549197 mentions this issue: doc/go1.22: document zero-copy to net.UnixConn

Change https://go.dev/cl/549335 mentions this issue: doc/go1.22: correct the system call name used for zero-copy from net.TCPConn to net.UnixConn