fifo: add new fifo package for named pipes #4665
Conversation
client/lib/fifo/fifo_windows.go
Outdated
| winio "github.com/Microsoft/go-winio" | ||
| ) | ||
|
|
||
| const PipeBufferSize = int(^uint16(0)) |
client/lib/fifo/fifo_unix.go
Outdated
| return Open(path) | ||
| } | ||
|
|
||
| func Open(path string) (io.ReadWriteCloser, error) { |
There was a problem hiding this comment.
Hm, should we make an OpenReader and OpenWriter API to avoid misuse? New could return a Reader as it should be the reader's responsibility to create the fifo as needed.
There was a problem hiding this comment.
Yeah I like this. The writer should also always be the one to close it I think so New and OpenReader will just return an io.Reader and OpenWriter can return a io.WriteCloser.
There was a problem hiding this comment.
Actually I don't think we will ever need an OpenReader. Plus it doesn't work on windows since creating the pipe returns a listener.
There was a problem hiding this comment.
The logger will read and write into the log file. And we'll need a writer for the driver-side.
The read side will close when the write side closes, so I don't think the reader will need to be a Close method but it wouldn't hurt. Might be useful for tests or future use cases.
The Windows and Unix APIs should match to avoid accidental build errors on the Windows side.
client/lib/fifo/fifo_unix.go
Outdated
| "syscall" | ||
| ) | ||
|
|
||
| func New(path string) (io.ReadWriteCloser, error) { |
client/lib/fifo/fifo_windows.go
Outdated
|
|
||
| const PipeBufferSize = int(^uint16(0)) | ||
|
|
||
| type FIFO struct { |
There was a problem hiding this comment.
Let's not export it so the Window's version doesn't export a different API from the non-Window's version.
client/lib/fifo/fifo_windows.go
Outdated
| } | ||
|
|
||
| func (f *FIFO) Close() error { | ||
| if f.conn != nil { |
There was a problem hiding this comment.
Conn.Close is threadsafe and File.Close not being threadsafe was considered a bug: golang/go#7970
Perhaps we should lock around the conn check? The only way to unblock a blocked Read() or Write() is to Close it.
client/lib/fifo/fifo_windows.go
Outdated
| } | ||
|
|
||
| // OpenWriter opens a fifo that already exists and returns a writer for it | ||
| func OpenWriter(path string) (io.WriteCloser, error) { |
There was a problem hiding this comment.
Why not just open and return a readwritecloser?
There was a problem hiding this comment.
Haha this is what I initially had but @schmichael thought having seperate funcs for Reader/Writer would be beneficial.
I settled on only having a writer here since New returns a reader. For our use, the data will be unidirectional so New would be the only place we want a reader.
This can totally be a ReaderWriterCloser, it just didn't make sense to me to ever need to read from the pipe (other than for the caller of New).
There was a problem hiding this comment.
Won't bike shed on this since it will do what we need but I would have both open/new return readwriteclosers
nickethier
force-pushed
the
r-clientv2
branch
from
ba95b44
to
2b03e8b
Compare
nickethier
force-pushed
the
f-fifo
branch
2 times, most recently
from
47f08b2
to
990c4ee
Compare
|
Added an additional test and ran them against windows |
client/lib/fifo/fifo_windows.go
Outdated
| } | ||
|
|
||
| // OpenWriter opens a fifo that already exists and returns a writer for it | ||
| func OpenWriter(path string) (io.WriteCloser, error) { |
There was a problem hiding this comment.
Won't bike shed on this since it will do what we need but I would have both open/new return readwriteclosers
* fifo: add new fifo package for named pipes
* fifo: add new fifo package for named pipes
* fifo: add new fifo package for named pipes
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
No description provided.