Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os: add File.SyscallConn method to permit frobbing file descriptor #24331

Closed
npat-efault opened this Issue Mar 10, 2018 · 25 comments

Comments

Projects
None yet
@npat-efault
Copy link
Contributor

npat-efault commented Mar 10, 2018

Since go 1.9, os.Files are backed by a mechanism similar to netpoll, whenever possible. This means that you can set read and write deadlines on them, and that you can safely Close() a file from another goroutine while Read() and / or Write() calls are active.

For working with device nodes (/dev/XXX files, like serial ports /dev/ttySx, but not only) this is very useful.

Sadly it turns out one cannot use this new feature, for a trivial and equally frustrating reason:

Occasionally one needs to access the underlying file-descriptor of a device in order to do a trivial ioctl() or something similar. But calling File.Fd(), sets the underlying filedes to blocking mode and disables (forever) all the cool poller support (deadlines, etc).

One cannot turn the filedes to nonblocking mode himself, because he would also need to set internal File / pfd attributes which he has no access to.

Alternatively, one cannot start with a filedes (i.e. use syscall.Open) and convert it to a file, using NewFile() since Files created with NewFile are not considered pollable.

So please: Provide a way to get a File's underlying filedes without setting it to non-blocking mode, and (most importantly) without loosing all the poller goodies. A new method like File.RawFd() would be perfectly adequate and trivial to implement.

I believe my case (accessing pollable device nodes) is exactly one of those for which the poller support was added to File's. So I should be able to use it.

@npat-efault

This comment has been minimized.

Copy link
Contributor Author

npat-efault commented Mar 10, 2018

An alternative solution would be for NewFile() to test if the given fd is in non-blocking mode, in which case it would convert it to a pollable file. Otherwise, it would convert it to a non-pollable file, as it, unconditionally, does today. Actually, doing both would be nice...

@npat-efault

This comment has been minimized.

Copy link
Contributor Author

npat-efault commented Mar 10, 2018

If you want, I could make the changes to os.NewFile() and / or add the File.RawFd() method myself and submit a patch...

@odeke-em

This comment has been minimized.

Copy link
Member

odeke-em commented Mar 11, 2018

@npat-efault thank you for filing and for articulating your motivations for this issue.

You might be interested in this issue #22939 that was accepted for Go1.11 in which the proposal was to add an API to construct an *os.File with a non-blocking file descriptor. Perhaps that issue might solve your problem without having to add another method, plus it is just waiting on implementation so perhaps you might be interested?

In regards to #24331 (comment)

An alternative solution would be for NewFile() to test if the given fd is in non-blocking mode, in which case it would convert it to a pollable file. Otherwise, it would convert it to a non-pollable file, as it, unconditionally, does today. Actually, doing both would be nice...

@ianlancetaylor had the exact same thoughts in #22939 (comment)

Perhaps if os.NewFile sees that the descriptor is already in non-blocking mode, it should try adding it to the poller.

And now to the question for clarity, your issue says
os: File.Fd() sets underlying filedes to non-blocking.
However the context of it is that we are setting filedes to blocking mode by invoking *File.Fd().

Did you mean to say instead either of these options below?

  • os: File.Fd() set underlying filedes to blocking mode
  • proposal: os: provide a method on *File to access filedes without setting it to blocking mode
  • proposal: os: add a *File.RawFd method
@as

This comment has been minimized.

Copy link
Contributor

as commented Mar 11, 2018

What about the users that rely on the current behavior of file.Fd()?

@npat-efault npat-efault changed the title os: File.Fd() sets underlying filedes to non-blocking os: File.Fd() sets underlying filedes to blocking Mar 11, 2018

@npat-efault

This comment has been minimized.

Copy link
Contributor Author

npat-efault commented Mar 11, 2018

Sorry, this was a typo. I fixed the title.

My proposal is:

  • Leave File.Fd() as is, since users may depend on it

and one or, better, both of:

  1. Change NewFile() to convert an already-nonblocking filedes to a pollable file
  2. Add a File.RawFd() method that returns the underlying filedes without any conversion from / to blocking / non-blocking

Technically 1 above could be said that it breaks backwards compatibility (in some sense), but if we agree that this break is not one we care about (the original behavior was not useful anyway), and if we agree that this is the way to go, I could post a (or two for 1 and 2) CLs in a couple of days or sooner.

If we don't want to break backward compatibility in no way whatsoever by changing the behavior of NewFile(), then we could define a NewFile1() with the new behavior (yes, it's not the prettiest thing, but if you are so strongly set about backwards compatibility, you have to abide a bit of ugliness).

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 11, 2018

Change https://golang.org/cl/100077 mentions this issue: os: NewFile called with nonblock fd, tries to return pollable file

@npat-efault

This comment has been minimized.

Copy link
Contributor Author

npat-efault commented Mar 11, 2018

Submitted CL https://golang.org/cl/100077 for your consideration...

@andybons andybons added this to the Go1.11 milestone Mar 11, 2018

@andybons

This comment has been minimized.

Copy link
Member

andybons commented Mar 11, 2018

/cc @ianlancetaylor for a decision

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Mar 26, 2018

I thought we'd done (1) ("Change NewFile() to convert an already-nonblocking filedes to a pollable file") already. /cc @ianlancetaylor

@spf13 spf13 added the NeedsFix label Mar 26, 2018

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Mar 26, 2018

Re (2) I think adding SyscallConn is probably the next step instead of RawFd, but we don't need that yet.

@npat-efault

This comment has been minimized.

Copy link
Contributor Author

npat-efault commented Apr 3, 2018

@rsc

I agree... though a bit cumbersome to use, SyscallCon() is the "right way" to provide access to the underlying fd for misc operations (ioctls and stuff). This is how it's done for network connections, and there is no reason for it to be different for other "connection-like" file-descriptors. Furthermore it provides synchronization against concurrent Close() calls (which is something the user should provide himself in the RawFd() case).

I could start looking into the implementation of this, if there are no other takers...

gopherbot pushed a commit that referenced this issue Apr 11, 2018

os: use poller when NewFile is called with a blocking descriptor.
If NewFile is called with a file descriptor that is already set to
non-blocking mode, it tries to return a pollable file (one for which
SetDeadline methods work) by adding the filedes to the poll/netpoll
mechanism. If called with a filedes in blocking mode, it returns a
non-pollable file, as it always did.

Fixes #22939
Updates #24331

Change-Id: Id54c8be1b83e6d35e14e76d7df0e57a9fd64e176
Reviewed-on: https://go-review.googlesource.com/100077
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@zx2c4

This comment has been minimized.

Copy link
Contributor

zx2c4 commented Apr 19, 2018

I'm currently having to use gross syscall.Select signaling to work around concurrent .Read() and .Close() when dealing with a /dev/net/tun that I call .Fd() on. With #22939 taken care of, having .RawFd() is the remaining issue before I can remove the syscall.Select hack.

@odeke-em

This comment has been minimized.

Copy link
Member

odeke-em commented May 3, 2018

I could start looking into the implementation of this, if there are no other takers...

@npat-efault all yours :) Thank you for offering to work on it!

@ianlancetaylor ianlancetaylor changed the title os: File.Fd() sets underlying filedes to blocking os: add File.SyscallConn method to permit frobbing file descriptor Jun 27, 2018

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jun 27, 2018

@odeke-em

This comment has been minimized.

Copy link
Member

odeke-em commented Nov 13, 2018

How's it going @npat-efault? Kindly paging you as the Go1.12 cycle is a couple of weeks away from ending.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Dec 20, 2018

Change https://golang.org/cl/155517 mentions this issue: os: add SyscallConn method for os.File

@robpike

This comment has been minimized.

Copy link
Contributor

robpike commented Dec 20, 2018

I won't vote against this because I don't want a fight, but I wish it didn't go in to the os package as it encourages ugly I/O patterns that until now were only available through the syscall package, where they belong (if anywhere). Promoting this to the os package endorses them, and that's what I object to.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Dec 20, 2018

Copying from the issue (should have replied here in the first place).

This is a pattern we already adopted for the net package. I don't see it quite as you do. The ugly I/O patterns still use the syscall package. I don't think we're really endorsing them. What we're doing here is permitting people to use the os package for normal use while dropping down to the syscall package when they have to, instead of saying that if they have one ugly syscall use then they have to use the syscall package for everything, and can never use the os package.

You're right of course that it is overly complicated but it's hard to just wish away the weird stuff that people want to do.

@mikioh

This comment has been minimized.

Copy link
Contributor

mikioh commented Dec 21, 2018

@ianlancetaylor,

Can you please clarify what's the scope of the API? Does the API cover only a file, I mean, a byte-sequence, allowing partial reads/writes on a message that indicates an EOF-like signal, or more widely including networking stuff?

This is a pattern we already adopted for the net package.

Well, not exactly the same. The net package takes more information, e.g., a network parameter that indicates a set of communication capabilities, from API users to ensure correct behavior on work with the kernel, and provides more information for fault localization.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Dec 21, 2018

@mikioh The API under discussion here already exists and is already used by the net package. It's https://golang.org/pkg/syscall/#Conn and https://golang.org/pkg/syscall/#RawConn. This issue is about implementing the same API in the os package, by adding a method to os.File that returns a syscall.Conn. You say that it's not exactly the same as what we do in the net package, but it is exactly the same.

@gopherbot gopherbot closed this in c0914d5 Dec 27, 2018

@zx2c4

This comment has been minimized.

Copy link
Contributor

zx2c4 commented Dec 27, 2018

Very happy to see this land. This should improve things for TUN/TAP implementations quite a bit. Thanks a lot.

@mikioh mikioh modified the milestones: Go1.13, Go1.12 Dec 28, 2018

@mikioh

This comment has been minimized.

Copy link
Contributor

mikioh commented Dec 28, 2018

@ianlancetaylor,

Thanks for the clarification. I see your comments "implementing the same API in the os package" and "exactly the same as what we do in the net package" and don't see a comment like "both package implementations are same." That means that os.File.SyscallConn and net.{TCP,UDP,IP,Unix}Conn.SyscallConn may return different error values and methods of syscall.RawConn may return different error values, depending on the package returning the syscall.RawConn.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Dec 28, 2018

@mikioh That is true: the error values are not the same. It's not obvious to me that it makes sense for them to be the same, since the net package error values return an address, in the net.OpError.Source field, which does not exist for the os package error values.

@cpuguy83

This comment has been minimized.

Copy link

cpuguy83 commented Jan 8, 2019

I see this is in the 1.12 milestone. Is there a plan to have this in the next beta or pushed to 1.13?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Jan 8, 2019

@cpuguy83 Yes, this is committed to tip and will be in the next beta release.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Jan 8, 2019

Change https://golang.org/cl/156839 mentions this issue: doc: go1.12: mention os.File.SyscallConn

gopherbot pushed a commit that referenced this issue Jan 8, 2019

doc: go1.12: mention os.File.SyscallConn
Updates #24331

Change-Id: I2d7c996bbe29d5b3922588e199a106eb722c02e6
Reviewed-on: https://go-review.googlesource.com/c/156839
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.