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

Remove the reuse for UNIX stream DNS lookup. #567

Merged
merged 1 commit into from
Jul 6, 2021

Conversation

madeye
Copy link
Contributor

@madeye madeye commented Jul 6, 2021

Our UNIX stream based DNS lookup doesn't support connection reuse.

@Mygod
Copy link
Contributor

Mygod commented Jul 6, 2021

My compiler complains that this commit cannot be found: https://github.com/zonyitoo/tokio/commit/47b76a5a

@madeye
Copy link
Contributor Author

madeye commented Jul 6, 2021

I got the compilation error as well, you may have to rebase to an old commit.

Our UNIX stream based DNS lookup doesn't support connection reuse.
@madeye madeye force-pushed the mlv/remove-unix-stream-reuse branch from 4669527 to c8fc85c Compare July 6, 2021 01:56
@zonyitoo
Copy link
Collaborator

zonyitoo commented Jul 6, 2021

My compiler complains that this commit cannot be found: zonyitoo/tokio@47b76a5

Just ignore it. I will fix it until tokio's PR was merged.

Copy link
Contributor

@Mygod Mygod left a comment

Choose a reason for hiding this comment

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

Can confirm this fixes everything. (I thought I was going to implement a DNS cache in shadowsocks-android for a moment so thank god!)

@zonyitoo
Copy link
Collaborator

zonyitoo commented Jul 6, 2021

The UNIX stream should report errors if the endpoint have been closed. Hmm.. Why it is the cause of shadowsocks/shadowsocks-android#2751 ?

Copy link
Collaborator

@zonyitoo zonyitoo left a comment

Choose a reason for hiding this comment

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

LGTM. But why?

@madeye
Copy link
Contributor Author

madeye commented Jul 6, 2021

Our android UNIX stream resolver doesn't support connection reuse yet.

I will use this PR to create a new release of shadowsocks-android as a hotfix. But for now, I think it's okay to keep it open for shadowsocks-rust.

@Mygod
Copy link
Contributor

Mygod commented Jul 6, 2021

There is no reason to support connection reuse on local sockets since the delay is negligible. Currently, shadowsocks-android should close the socket (syscall close) after the response is written.

EDIT: Holding a local socket open also wastes system resources methinks.

@zonyitoo
Copy link
Collaborator

zonyitoo commented Jul 6, 2021

Well, you are right about the cost of creating unix local stream is negligible. I am Ok with this PR, but I still cannot understand why it cause that issue.

Currently the implementation will open a new client if the connection have been lost due to for _ in 0..self.retry_count.

@Mygod
Copy link
Contributor

Mygod commented Jul 6, 2021

Maybe we merge this and figure out the retry later? 😛

@zonyitoo
Copy link
Collaborator

zonyitoo commented Jul 6, 2021

I am worrying about if there are any issues in this connection management strategy, then it will also affect TCP and UDP DNS clients.

@zonyitoo
Copy link
Collaborator

zonyitoo commented Jul 6, 2021

Will shadowsocks-android implement connection reuse in the future? @madeye @Mygod

  • If NO, then I will merge this PR and only checks TCP and UDP connections;
  • If YES, then I will prefer another solution that checks the status of connection before actually write to it

@Mygod
Copy link
Contributor

Mygod commented Jul 6, 2021

There is no reason to support connection reuse on local sockets since the delay is negligible. Currently, shadowsocks-android should close the socket (syscall close) after the response is written.

EDIT: Holding a local socket open also wastes system resources methinks.

^

@zonyitoo zonyitoo merged commit d4b1057 into master Jul 6, 2021
zonyitoo added a commit that referenced this pull request Jul 6, 2021
zonyitoo added a commit that referenced this pull request Jul 6, 2021
@zonyitoo zonyitoo deleted the mlv/remove-unix-stream-reuse branch July 6, 2021 16:44
zonyitoo added a commit that referenced this pull request Jul 6, 2021
atkdef pushed a commit to atkdef/shadowsocks-rust that referenced this pull request Sep 24, 2021
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.

None yet

3 participants