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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

New local DNS server implemented with Kotlin coroutines #2096

Merged
merged 59 commits into from Feb 11, 2019

Conversation

Projects
None yet
2 participants
@Mygod
Copy link
Contributor

Mygod commented Jan 29, 2019

馃毀 DO NOT MERGE 馃毀 No longer in construction but in active testing

Supersedes Mygod#2.

Nothing works yet (well actually receiving DNS and local resolver work) and somebody just needs to debug this I guess.

Also I believe Kotlin coroutines have comparable performance with goroutines [1] [2] [3], but obviously that needs fine tuning. The current code probably runs a little slow but again code in overture was not written with great care either.

[With this PR, we] can also respect local DNS settings (fix #2016), and therefore including possibly support private DNS on Android 9+ (closes #2015).

P.S. It seems like private DNS is not working on my side with Wi-Fi + VPN but works fine with mobile data + VPN. Can anyone also confirm this?

Update: #2096 (comment)

@Mygod Mygod changed the base branch from dnsproxyd to master Jan 29, 2019

@Mygod Mygod added the enhancement label Jan 29, 2019

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Jan 29, 2019

@madeye Do you think this would be a good idea at all (assuming all bugs fixed)? There is also an alternative approach, which is to pass network handle of underlyingNetwork to overture and overture will call android_getaddrinfofornet. That way I believe there is also no need to use protect socket at all for overture. (but I really don't like including a heavy golang project fork if it can be done with <200 lines of Kotlin)

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Jan 29, 2019

I think it depends on the effort we need to implement a kotlin local DNS server.

If we can solve the problem with existing software and much less effort, it'd be better try it first.

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Jan 29, 2019

Everything fixed (I believe, including the UDP DNS SOCKS5 forwarder which was not used). Still <200 lines. 馃槅

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Jan 29, 2019

Now the universal apk is down to ~6MB (split apk ~5MB), also CircleCI builds.

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Jan 29, 2019

Cool! I will do more tests later.

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Jan 29, 2019

There are still a few glitches with withTimeout it seems. Meanwhile, I am trying to rewrite this (yet again) using non-blocking IO (java.nio.channels) to see if we can get around that.

I should probably stop changing stuff in this repo every two days and focus on other work... Please send help.

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Jan 29, 2019

No hurry, it's not an urgent task. 馃

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Jan 30, 2019

Cool! It works quite well on my local device.

BTW, I didn't see any glitch locally. Maybe just decrease the timeout here.

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Jan 30, 2019

Well, the glitch is that sometimes service will be stuck in STOPPING state. I only encountered it once though and I'm not sure if decreasing timeout would help.

@Mygod Mygod referenced this pull request Jan 30, 2019

Closed

DNS server NIO #2097

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Jan 31, 2019

According to my profiler, the Java heap after GC is ~9MB, so I expect this change should decrease the memory size of background processes (top shows that liboverture.so uses 10MB in RES. No matter how efficient is golang, you still need to load the binary into the memory after all). Of course, in comparison, each DNS request now needs more temporary memory in JVM to serve (according to my calculation, ~50KB), but overall I believe this is a welcoming change for retained memory size.

Let's run this for an extensive period of time and see if there are memory leaks (and any other possible problems). If not, I think we can merge this.

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Feb 8, 2019

BTW, all the tests are done with v2ray plugin.

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Feb 8, 2019

The key to reproducing issue of 00dfb92 is locking the phone. So, it looks related to doze?

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Feb 8, 2019

Interesting, looks 493b6e8 fixed all the issues.

Let me do more tests locally.

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Feb 8, 2019

It's probably helpful to dump the thread stacks for debugging when that happens: https://stackoverflow.com/q/13589074/2245107

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Feb 8, 2019

BTW 493b6e8 actually fixes bugs introduced by 490e043.

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Feb 8, 2019

@madeye f4e65b2 and 9894d0e should fix occasional deadlocks on connecting.

There are quite a few commits in the last few hours that should have been committed to master branch instead, but I am too lazy to switch branches and do merges. 馃槄

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Feb 9, 2019

@madeye Any updates? It is running super smoothly for me for now.

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Feb 10, 2019

Yeah, i think all the issues have been resolved, including previous doze problems.

Mygod added some commits Feb 10, 2019

@Mygod
Copy link
Contributor Author

Mygod left a comment

Ready to merge?

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Feb 11, 2019

Yeah, I think so.

@Mygod

This comment has been minimized.

Copy link
Contributor Author

Mygod commented Feb 11, 2019

No complaints heard over the past 3 days. I am pretending that this is good enough because YOLO! 馃ぃ

@Mygod Mygod merged commit daad494 into shadowsocks:master Feb 11, 2019

1 check failed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details

@Mygod Mygod deleted the Mygod:dns-server branch Feb 11, 2019

@madeye

This comment has been minimized.

Copy link
Contributor

madeye commented Mar 1, 2019

It looks the deep sleep problem happened again.

With the latest commit, the DNS resolver died after hours of standby.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.