search

shadowsocks / shadowsocks-windows

A C# port of shadowsocks
Other
58.33k stars 16.39k forks source link

Discuss PR (mainly about IE proxy) #186

Closed krrr closed 9 years ago

krrr commented 9 years ago

compare You said "...make a significant change..." in CONTRIBUTING, maybe this one is. I worked out this problem, but I also made some changes that I like. It works well in my machine.

For IE proxy, I imported a library that help dealing with win32api and C++ structures.

For REUSEADDRESS problem, this option has very different meaning in windows compared to UNIX. Why you use this option?

clowwindy commented 9 years ago

First thanks for the great job. You have solved your problem.

Before we can merge this patch, we have to understand why it works. I saw you have borrowed a large piece of code from https://github.com/chentiangemalc/ProxyUtils. But this is not enough for a production project. We don't just borrow large pieces of code from somewhere and put them together. Instead, we learn from other projects, and write our own code.

I guess you can start from remove part of the code to see if it still works. Then you'll know what exactly is missing in the original code, and why it leads to your problem.

After that, make the minimal change required to the original code to fix your problem. You may also add some comments to explain the problem and how you fixed it.

And try to fix only one thing in one pull request. You may want to split your changes into several small pull requests.

Others changes I saw:

  1. Disable polipo if system proxy is not enabled

    Well, this is a common misunderstanding. Because some people still want an HTTP proxy, for example, for Google Drive client which doesn't support Socks.

  2. ReuseAddress

    This option is for the new process to bind on the same port even before the old process is fully destroyed.

krrr commented 9 years ago

You said I sovled my problem, is that means in your(and most people's) machine this problem does not exists?

About IE proxy: I debugged the program and found that after register keys under HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Internet Settings\ changed, InternetSetOption call with INTERNET_OPTION_REFRESH (the second with this option) will reset those register keys (corresponding to \Connections\DefaultConnectionSettings). The original way is almost a hack, so strange problem is not so strange.

Disable polipo will also break the update checker, I have not noticed that...

ReuseAddress can make your program bind to the port which other process are currently using in windows. And I tried to kill shadowsocks while transferring data, then quickly restart it, no problem happen (I modified it to use ExclusiveAddressUse)?

clowwindy commented 9 years ago

Yes, it works well on my machine, and probably most people's. So what's the minimal lines of code to add to solve the problem, without using the entire code block from another project?

krrr commented 9 years ago

No, maybe I have to find out what the problem my OS has.

krrr commented 9 years ago

I failed to find a solution to patch the "poking registry way", but someone said this way only fail on windows 7.