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

Threadsafety #128

Closed
mappu opened this issue Oct 30, 2018 · 1 comment
Closed

Threadsafety #128

mappu opened this issue Oct 30, 2018 · 1 comment
Labels
accepted Accepted issue

Comments

@mappu
Copy link
Contributor

mappu commented Oct 30, 2018

I think it's currently unsafe to use a ServerConn from multiple goroutines because of a race in ServerConn.cmd between Cmd and ReadResponse.

Can this be documented?

Better yet, can it be fixed? If the control connection is protected by a mutex, do FTP servers permit you to have multiple pasv data connections open simultaneously?

@jlaffaye jlaffaye added the accepted Accepted issue label Nov 1, 2018
@hbobenicio
Copy link

A general rule of thumb is:

"Do not communicate by sharing memory; instead, share memory by communicating."

(https://blog.golang.org/share-memory-by-communicating)

So I think that maybe the issue here is not that the ServerConn is not thread safe. The issue maybe... why you are sharing a connection between goroutines without channeling messages? And... do you really need that?

This would be my approach for multiple gouroutines needing the use of a ServerConn:

  1. More naive and simple and, depending on what your trying to do, also expensive: Open one connection per goroutine. Do not share connections, but share it's processing result with channels.
  2. More complex and, depending on what your trying to do, performant: Create a connection pool for your ServerConn. Create a goroutine for managing it (if the pool library used do not already provides that) in a loop which the other goroutines could send message for and demand accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Accepted issue
Projects
None yet
Development

No branches or pull requests

3 participants