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

net: clarify net.Conn concurrency requirement #27203

Open
FiloSottile opened this issue Aug 24, 2018 · 16 comments
Open

net: clarify net.Conn concurrency requirement #27203

FiloSottile opened this issue Aug 24, 2018 · 16 comments
Labels
Documentation help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@FiloSottile
Copy link
Contributor

From the net.Conn docs:

Multiple goroutines may invoke methods on a Conn simultaneously.

I'm not sure if this should be interpreted as different methods can be called concurrently, or as the same method can be called concurrently.

I suspect, judging from x/net/nettest's ConcurrentMethods and from most implementations, that it's the former. If so, should we change the docs to include "may invoke distinct methods"?

@FiloSottile FiloSottile added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 24, 2018
@FiloSottile FiloSottile added this to the Go1.12 milestone Aug 24, 2018
@ianlancetaylor
Copy link
Contributor

It's the former: any methods may be called concurrently. But changing the docs to say distinct methods would suggest that you could not call the same method concurrently--e.g., two concurrent calls to Write.

Personally I think the current text is clear but I'm open to counter arguments.

@FiloSottile
Copy link
Contributor Author

Calling the same method concurrently however is not part of the interface contract. Implementations might allow it, but don't have to. With the current text, it's not clear to me if a net.Conn implementation that breaks for concurrent calls to Read is correct.

@ianlancetaylor
Copy link
Contributor

I don't understand. The text is, as you say, "Multiple goroutines may invoke methods on a Conn simultaneously." To me that clearly states that multiple goroutines may invoke the Read method simultaneously. Are you saying that that does not work?

@FiloSottile
Copy link
Contributor Author

Ah, I interpreted your first message the other way around.

That seems more strict than what I've seen implemented in practice, but I'm willing to believe it.

x/net/nettest's ConcurrentMethods should test for it then, though, because it looks like it only tests concurrent calls of distinct methods.

@bradfitz
Copy link
Contributor

Also, some implementations may promise more: e.g. UDPConn permitting concurrent Write calls.

@networkimprov
Copy link

networkimprov commented Aug 25, 2018

Doesn't TCPConn allow simultaneous Write() to a single net.Conn? The docs certainly imply that.

I make TCPConn.Write calls from one goroutine while another does io.Copy (invoking sendfile()), and haven't seen any errors that indicated "connection in use".

Pls do test this formally if possible :-)

@gopherbot add Testing

@FiloSottile
Copy link
Contributor Author

@bradfitz I'm confused again, AFAICT @ianlancetaylor is saying that all net.Conn implementations must permit concurrent Write calls, so it wouldn't be promising more.

I'll argue that we should make the docs more explicit after all.

@gopherbot gopherbot added the Testing An issue that has been verified to require only test changes, not just a test failure. label Aug 26, 2018
@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Unplanned Dec 18, 2018
@FiloSottile FiloSottile removed the Testing An issue that has been verified to require only test changes, not just a test failure. label Sep 23, 2019
@bercknash
Copy link

As a C programmer new to go, I also find the statement "Multiple goroutines may invoke methods on a Conn simultaneously" to be a bit ambiguous. It makes sense that it's probably safe to simultaneously call Read() and Write() from separate goroutines, but it's less clear what's going to happen if, say, I call conn.Write() from several goroutines simultaneously. I take the statement to mean that I "may" do it, but if I do, does my data get randomly interleaved? Or is it truly threadsafe and simultaneous conn.Write() commands will block while the others are writing?

@networkimprov
Copy link

@FiloSottile you removed the Testing label. Does that mean tests have been added for concurrent calls of the same methods?

If not, we should create an issue to request that...

@FiloSottile
Copy link
Contributor Author

Ah, my bad, I didn't get why it was tagged Testing. I still don't have clear what the interface contract is, so it feels like first we need to update the docs, and then the tests.

@FiloSottile FiloSottile added the Testing An issue that has been verified to require only test changes, not just a test failure. label Sep 23, 2019
@ianlancetaylor
Copy link
Contributor

@bercknash A single Write call will acquire a write lock on the net.Conn until the Write is complete or has an error. So concurrent Write calls are permitted and will not interleave.

@bercknash
Copy link

bercknash commented Sep 24, 2019

@bercknash A single Write call will acquire a write lock on the net.Conn until the Write is complete or has an error. So concurrent Write calls are permitted and will not interleave.

Thanks for clarifying. I gathered as much from digging through the code, but it's the sort of thing I think the docs should make explicitly clear without having to dive in.

@networkimprov
Copy link

There is another ambiguity the docs could clarify. io.Copy(conn, src) is atomic in some cases, and not in others.

An io.Copy to a net.TCPConn is atomic, (on Linux at least) because it calls sendfile(2) internally. But to a tls.Conn it's not atomic, as that may do a sequence of c.Write ops.

I was bitten by this when changing from a TCPConn to TLS :-)

@eloff
Copy link

eloff commented May 19, 2021

@bercknash A single Write call will acquire a write lock on the net.Conn until the Write is complete or has an error. So concurrent Write calls are permitted and will not interleave.

This is why the docs could use some clarification, the statement "Multiple goroutines may invoke methods on a Conn simultaneously" is ambiguous. Safe might mean calls are atomic and don't interleave, or it might only mean calls don't crash or corrupt internal state. See this stackoverflow answer here which specifically interprets it to mean there is no guarantee: https://stackoverflow.com/a/37922991/152580

I also interpreted it that way until I read the code. It would be nice to have a clearer guarantee of the implemented behaviour for net.Conn methods.

To answer @davecheney from the duplicate issue I opened:

What’s the use case for having two or more goroutines writing to the same net.Conn concurrently?

I've got a case where there can be messages produced asynchronously (from another goroutine) or synchronously. It doesn't matter what order async messages are sent, it could be before or after the sync messages, as long as their bytes don't interleave.
Without atomic Write I would need to acquire a mutex around sending the messages.

@eloff
Copy link

eloff commented May 19, 2021

There is another ambiguity the docs could clarify. io.Copy(conn, src) is atomic in some cases, and not in others.

An io.Copy to a net.TCPConn is atomic, (on Linux at least) because it calls sendfile(2) internally. But to a tls.Conn it's not atomic, as that may do a sequence of c.Write ops.

I was bitten by this when changing from a TCPConn to TLS :-)

Also net.Buffers WriteTo would be atomic on platforms with writev or equivalent, but not others, and not with TLS connections. I don't think people should expect those to be atomic though, and nothing about their documentation leads me to believe that they would be safe to call concurrently.

@betamos
Copy link

betamos commented Apr 22, 2024

I'd like to see clarification of this as well. There are some nasty bugs and data races that can occur without proper care.

Note that TCP- and TLS conns, while very common, are not the only ones to care about. If you implement a wire protocol, it's common and extremely useful to implement net.Conn for interop. Typically, you'd use a bufio.Reader and override Read, and similarly bufio.Writer for writes, while leaving piping other methods like Set*Deadline, *Addr and Close to the underlying conn. This works well in practice and is easy to do.

Conforming with the most stringent thread-safety definition (which allows e.g. multiple concurrent write calls), would mean that any wire protocols need heavily fortify standard IO with extra mutices, because underlying bufio.* types and similar are not thread-safe. I suspect a lot of wire protocols skip this, because it's wasteful, error prone and usually an application-level error anyway. Or they simply forget about it, because thread safe wrapping is not standard practice.

Anyway, I'd argue thread safety should only be enabled for good reason:

  • Read can be concurrent with Write (enables full duplex)
  • Set*Deadline can be concurrent with reads and write (unblocks io)
  • Close can be concurrent with all of the above (unblocks io)

Most of this is already documented, and it's pretty much what people expect already. It does however need some clarification.

In the existing cases where there are stronger guarantees, such as for UDPConn concurrent writes(?), then those can be specified separately.

Finally, if the Go team agrees with this, then it's worth noting that x/net/nettest which is a useful test suite for wire protocol implementations, currently checks for "racy writes and reads".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

9 participants