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

take arbitrary IO for udp proxy #1641

Merged
merged 6 commits into from
Sep 20, 2024
Merged

Conversation

ibigbug
Copy link
Contributor

@ibigbug ibigbug commented Sep 3, 2024

@ibigbug ibigbug marked this pull request as draft September 3, 2024 13:09
@zonyitoo
Copy link
Collaborator

zonyitoo commented Sep 4, 2024

Why was that necessary? SocketAddr is also implements ToSocketAddrs.

@ibigbug
Copy link
Contributor Author

ibigbug commented Sep 4, 2024

We need it to be SocketAddr when passing it to the underlying udp send

@zonyitoo
Copy link
Collaborator

zonyitoo commented Sep 4, 2024

Why was that necessary? SocketAddr is also implements ToSocketAddrs.

What was the key issue of the current master implementation that blocked you from passing SocketAddr to the APIs?

@ibigbug
Copy link
Contributor Author

ibigbug commented Sep 4, 2024

There is no issue passing a SocketAddr, but we can't and shouldn't pass IntoSocketAddrs into an abstract udp up, which is not always a tokio udpsocket

@ibigbug
Copy link
Contributor Author

ibigbug commented Sep 5, 2024

just realised it's a bit tricky to have the abstract io to be compatible with the raw socket operations. maybe I should separate two kinds of io and fail all the raw socket operation on the abstract one and give the control of that back to the author of which

@zonyitoo
Copy link
Collaborator

zonyitoo commented Sep 5, 2024

Please resolve all the CI failures.

@ibigbug
Copy link
Contributor Author

ibigbug commented Sep 5, 2024

Sure. There are still more to do. Will update soon.

@ibigbug ibigbug marked this pull request as ready for review September 10, 2024 14:16
@ibigbug
Copy link
Contributor Author

ibigbug commented Sep 10, 2024

@zonyitoo this should be ready for review now

@zonyitoo
Copy link
Collaborator

zonyitoo commented Sep 18, 2024

I was wondering another solution:

  1. Make a trait DatagramTransport, which has APIs: send, recv, send_to, recv_from.
  2. ProxySocket took a type impls DatagramTransport as the underlying I/O socket
  3. UdpSocket impls DatagramTransport, which will require no lock or any other synchronization method in the application level.
  4. You can impls your customized transport by impls DatagramTransport to support Sink and Stream.

@zonyitoo
Copy link
Collaborator

I could apply my ideas on your branch, if you allowed.

@ibigbug
Copy link
Contributor Author

ibigbug commented Sep 19, 2024

yeah of course, feel free to update

@zonyitoo
Copy link
Collaborator

zonyitoo commented Sep 19, 2024

@ibigbug Please set your branch to be "allow editable by maintainer".

@ibigbug
Copy link
Contributor Author

ibigbug commented Sep 20, 2024

@zonyitoo i odn't see that option, maybe it's from a forked repo?

@zonyitoo
Copy link
Collaborator

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

On user-owned forks, if you want to allow anyone with push access to the upstream repository to make changes to your pull request, select Allow edits from maintainers.

@ibigbug
Copy link
Contributor Author

ibigbug commented Sep 20, 2024

image nothing shows up here?

@ibigbug
Copy link
Contributor Author

ibigbug commented Sep 20, 2024

https://github.com/orgs/community/discussions/5634

seems it doesn't work for org

@zonyitoo zonyitoo self-assigned this Sep 20, 2024
@zonyitoo
Copy link
Collaborator

I may have to modify it on the master branch after merging this PR.

@zonyitoo zonyitoo merged commit 4e29581 into shadowsocks:master Sep 20, 2024
9 checks passed
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