Skip to content

Disable allow_reuse_address under some conditions #418

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Disable allow_reuse_address under some conditions #418

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions msal/oauth2cli/authcode.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"""
import logging
import socket
import sys
from string import Template
import threading
import time
Expand Down Expand Up @@ -144,6 +145,26 @@ def __init__(self, port=None):
Server = _AuthCodeHttpServer6 if ":" in address else _AuthCodeHttpServer
# TODO: But, it would treat "localhost" or "" as IPv4.
# If pressed, we might just expose a family parameter to caller.

if port:
# 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
Comment on lines +150 to +160
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.

else:
# 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.
Comment on lines +162 to +163
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.

# Also, since an ephemeral port is chosen, TIME_WAIT and SO_LINGER won't be a problem.
# Therefore, allow_reuse_address should be disabled.
Server.allow_reuse_address = False

self._server = Server((address, port or 0), _AuthCodeHandler)
self._closing = False

Expand Down