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

Disable allow_reuse_address under some conditions #418

Closed
wants to merge 1 commit into from

Conversation

jiasli
Copy link
Contributor

@jiasli jiasli commented Oct 15, 2021

acquire_token_interactive has a port parameter.

When port is explicitly specified, like 8400

On non-Windows platforms (Linux, FreeBSD, etc.)

allow_reuse_address should be set to True, in order to avoid TIME_WAIT and SO_LINGER problem (https://stackoverflow.com/a/14388707/2199657).

This is the default behavior of HTTPServer and why allow_reuse_address gets set to True in the first place (python/cpython@18865de).

On Windows

This also allows port reuse, making multiple MSAL instances be able to listen to the same port (https://stackoverflow.com/a/14388707/2199657, Azure/azure-cli#10578).

On the other hand, Windows doesn't seem to have TIME_WAIT and SO_LINGER problem by default without SO_EXCLUSIVEADDRUSE (tornadoweb/tornado#550).

Therefore, allow_reuse_address should be disabled (Azure/azure-cli#10955).

When ephemeral port 0 is used

allow_reuse_address may cause an in-use port to be returned, causing EADDRINUSE long before the ephemeral port space has been exhausted (https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=174087).

Also, since an ephemeral port is chosen, TIME_WAIT and SO_LINGER won't be a problem.

Therefore, allow_reuse_address should be disabled (https://gavv.github.io/articles/ephemeral-port-reuse/).

Related discussions

Copy link
Collaborator

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Thanks Jiashuo. I am leaving some comments. You do not have to actually make those code changes. You can leave your feedback, and then I can follow up by adding more commits into your feature branch.

Comment on lines +162 to +163
# When ephemeral port 0 is used, allow_reuse_address may cause an in-use port to be returned,
# causing EADDRINUSE long before the ephemeral port space has been exhausted.
Copy link
Collaborator

Choose a reason for hiding this comment

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

allow_reuse_address may cause an in-use port to be returned, causing EADDRINUSE long before the ephemeral port space has been exhausted (https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=174087).

That article seemed to suggest that the issue would only be observable when the client-side creates lots of sockets: "Setting SO_REUSEADDR before calling connect() has some surprizing consequences". Its test program also only creates lots of socket in its cycle_client().

Therefore, allow_reuse_address should be disabled (https://gavv.github.io/articles/ephemeral-port-reuse/).

This article provided same result. In particular, its test program and test result suggested that, if listen() was being called (as in the server-side listening for incoming socket), the port conflict would not be observed.

So, perhaps we can say there is not enough evidence to avoid SO_REUSEADDR on an ephemeral port in a socket server.

Comment on lines +150 to +160
# When port is explicitly specified, like 8400:
# On non-Windows platforms (Linux, FreeBSD, etc.), allow_reuse_address should be set to True,
# in order to avoid TIME_WAIT and SO_LINGER problem. This is the default behavior of HTTPServer,
# and why allow_reuse_address gets set to True in the first place.

# On Windows, this also allows port reuse, making multiple MSAL instances be
# able to listen to the same port. On the other hand, Windows doesn't seem to have
# TIME_WAIT and SO_LINGER problem by default without SO_EXCLUSIVEADDRUSE.
# Therefore, allow_reuse_address should be disabled.
if sys.platform == "win32" or is_wsl():
Server.allow_reuse_address = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for bringing this to our attention. It seems all those TIME_WAIT, SO_LINGER, and SO_EXCLUSIVEADDRUSE topics (on Windows or not) are so intensive. :-) In our case, one reason (derived by Azure/azure-cli#10578) is convincing enough for us to make some change: running python3 -m http.server twice on Windows, the second run would seemingly also proceed (but would not actually receive any incoming request). That is the problem we would want to solve by allow_reuse_address = False. The new expected behaviour would be an exception being raised. And then we can and probably should come up with a unit test case for it.

Implementation wise, perhaps we should move this part of logic into the underlying class _AuthCodeHttpServer.

class _AuthCodeHttpServer(HTTPServer):
    def __init__(self, server_address, RequestHandlerClass, **kwargs):
        _, port = server_address
        if port and (sys.platform == "win32" or is_wsl()):
            # On Windows, the default allow_reuse_address is True,
            # which would undesirably allow multiple server listening on same port.
            self.allow_reuse_address = False
        super(...)...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course. Please feel free to branch off or directly edit my branch as you see fit.

@rayluo
Copy link
Collaborator

rayluo commented Oct 28, 2021

The new expected behaviour would be an exception being raised. And then we can and probably should come up with a unit test case for it.

Implementation wise, perhaps we should move this part of logic into the underlying class _AuthCodeHttpServer.

This part is now implemented in #427. The rest of this PR might not be necessary. Closing this PR, for now. If we feel a need to revisit this topic, we can rebase/reopen this PR later.

Thanks again for @jiasli 's research to bring up this valuable topic in the first place!

@rayluo rayluo closed this Oct 28, 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.

2 participants