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

Improved userspace tunnel concurrency to avoid race conditions. #503

Merged

Conversation

stoktamisoglu
Copy link
Contributor

The problem is described in the issue #502. A potential solution to this problem is to close the reader and writer after the io,Copy() and sending result (EOF, error) to the error channel. By closing the writer and reader the counter part io.Copy() will drain and unblocked. By this change the code will be resistant to one side network connection closes.

The close function needs to be synced with sync.Once to avoid multiple entrance so double freeing.

Apart from this it is essential to use buffered channel like

errCh := make(chan error, 2)

Because, buffered channel sender and receiver should be synced at the same time. This means if the code doesn't reach

return errors.Join(<-err1, <-err2)

at the same time with the line in ioCopyWithErr

errCh <- err

Then it will be dead/circular locked.

I've tested it with different scenarios as mentioned in #502 and it worked very performant and with no locking issue at all.

I'm not a golang expert but after some extensive research this solution seems robust.

…ed error channel with the buffered channels for unblocking send and receive.
@stoktamisoglu
Copy link
Contributor Author

stoktamisoglu commented Nov 6, 2024

Hi @danielpaulus @dmissmann @shamanec , would you please review the proposed fix as it fixed several problems when the tunnel is overwhelming particularly in a loop to launch ios with launchapp, killapp, ps and apps.
Thanks in advance for your time.

Copy link
Owner

@danielpaulus danielpaulus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stoktamisoglu this is an amazing fix! Thanks for the great writeup and PR description.

@danielpaulus danielpaulus merged commit 6bc43a1 into danielpaulus:main Nov 6, 2024
2 checks passed
@stoktamisoglu
Copy link
Contributor Author

Thanks a lot @danielpaulus for prompt reviewing and new release.

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

Successfully merging this pull request may close these issues.

2 participants