cleanup: refactor and use bicopy everywhere #2944
Conversation
tamird
force-pushed
the
remove-bicopy
branch
2 times, most recently
from
4ec7fa0
to
63ae319
Compare
There was a problem hiding this comment.
This changes the behavior but you did not provide any reason for the change. If the purpose is to remove eliminate a package, just inline the package code in the single user. There is no need to change the code.
If you want to reimplement it and/or change the behavior, please explain why.
Also we have good tests for port_darwin? Before rewriting we should have good tests ensuring that we don't change the behavior by mistake. It should be simple test coping data over unix sockets.
tamird
force-pushed
the
remove-bicopy
branch
2 times, most recently
from
6a612d9
to
c8e7582
Compare
tamird
force-pushed
the
remove-bicopy
branch
3 times, most recently
from
a2a75a4
to
b92ac24
Compare
tamird
changed the title
There are 3 instances of this pattern and each does this slightly differently. Clean up the implementation to return errors using `errors.Join` (which wasn't available when the original was written) and use it everywhere. This doesn't change behavior because the error return is always just logged (see the only called of `(*pseudoLoopbackForwarder).forward`. Note that the removal of the special handling of `io.EOF` returned from `io.Copy` doesn't change behavior because it can never happen per the latter's documentation. Signed-off-by: Tamir Duberstein <[email protected]>
tamird
force-pushed
the
remove-bicopy
branch
from
b92ac24
to
3d32079
Compare
| return errors.Join( | ||
| errCopy, | ||
| closeWrite(dst, dstName), | ||
| closeRead(src, srcName), |
There was a problem hiding this comment.
Why do we need to do CloseRead() and CloseWrite() after io.Copy()?
Why we we join all the errors here? If the copy failed, we don't care about CloseRead() or CloseWrite() failure, and joining all the errors only makes it harder to understand the error later. We may have 3 errors from each call, total 6 errors are possible.
| go func() { | ||
| wg.Wait() | ||
| close(finish) | ||
| errLeft = broker(left, right, leftName, rightName) |
There was a problem hiding this comment.
errLeft is not clear name for this io.Copy.
| errRight, | ||
| ioClose(left, leftName), | ||
| ioClose(right, rightName), | ||
| ) |
There was a problem hiding this comment.
Here we may get 8 errors joined. This is not helpful.
I don't understand what is the purpose of this change. Using the same infra for similar operation makes sense, but what is wrong with the current code, for example in vz/network_darwin.go? Why do you want to change this code?
The current code in vz/net_darwin.go is simple and easy to understand and produce good and useful logs on errors. If you want to change it lets start with an issue explaning the issue in this code. Same for the similar code in portfwd and hostagent.
What is suggested in this change is much more complicated and the joined errors are a mess.
| wg.Wait() | ||
| if err := bicopy.Bicopy(qemuConn, vzConn, "VMNET", "VZ"); err != nil { | ||
| logrus.WithError(err).Error("failed to forward packets") | ||
| } |
There was a problem hiding this comment.
Previously we logged the error immediately when it happens. Now we my log no errors since after one copy fail, the second may be blocked forever. This will make it harder or impossible to debug issues.
I'm not sure that the current code is ensuring that if one copy fail the other one will also fail, and we don't have test for this error case. Before replacing the code lets understand if this is an issue and how we can fix it.
When we have 3 correct implementations we may consider unifying them.
| _, err := io.Copy(rw, conn) | ||
| if errors.Is(err, io.EOF) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
For this code, I would start with removing the handling of io.EOF if we think that it is not possible.
|
|
||
| if err := g.Wait(); err != nil { | ||
| logrus.Debugf("error in tcp tunnel for id: %s error:%v", id, err) |
There was a problem hiding this comment.
The previous code tried to log both errors, assuming that if one copy fail, the other will fail soon, but this may not be the case. This may never log any error if the first io.Copy() fails and the other is stuck on blocking socket.
A better way to handle this is to use the same code we have in vz/network_darwin.go, logging the first error immediately.
There was a problem hiding this comment.
I did not review the previous bicopy.go code. What are the issues in the previous code that you are trying to fix?
See commit message.