Skip to content

Commit

Permalink
Merge remote-tracking branch 'oauth2cli/dev' into ctrlc
Browse files Browse the repository at this point in the history
  • Loading branch information
@rayluo
rayluo committed Sep 10, 2021
2 parents 2b7fa44 + 1fd1754 commit 0deba2e
Showing 1 changed file with 4 additions and 1 deletion.
5 changes: 4 additions & 1 deletion msal/oauth2cli/authcode.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ def __init__(self, port=None):
# TODO: But, it would treat "localhost" or "" as IPv4.
# If pressed, we might just expose a family parameter to caller.
self._server = Server((address, port or 0), _AuthCodeHandler)
self._closing = False

def get_port(self):
"""The port this server actually listening to"""
Expand Down Expand Up @@ -258,7 +259,8 @@ def _get_auth_response(self, result, auth_uri=None, timeout=None, state=None,

self._server.timeout = timeout # Otherwise its handle_timeout() won't work
self._server.auth_response = {} # Shared with _AuthCodeHandler
while True:
while not self._closing: # Otherwise, the handle_request() attempt

This comment has been minimized.

Sign in to view

Copy link
@jiasli

jiasli Sep 13, 2021

Contributor

Unfortunately, this is not a watertight solution. Consider when this line (L262) evaluates to True but the main thread closes the socket after this line is executed. L266 will then fail with ValueError. Using try except statements (#405) is a better solution according to Python's Easier to ask for forgiveness than permission principal.

Some discussion on Stack Overflow: https://stackoverflow.com/questions/1586648/race-condition-creating-folder-in-python

This comment has been minimized.

Sign in to view

Copy link
@rayluo

rayluo Sep 14, 2021

Author Collaborator

Documenting our offline discussion:

  • If the server.handle_request() would emit a specific exception, we would happily write our logic base on it. But it emits a generic ValueError, and we do not want to absorb/mask ValueError.
  • The current implementation self._closing flag already works on most of the time. The only chance to see race condition is to press CTRL+C immediately (like within 0.1 second?) right after the end user starts the acquire_token_interactive(). Even at that time, there is no harm other than a noisy error trace.
# would yield noisy ValueError trace
# Derived from
# https://docs.python.org/2/library/basehttpserver.html#more-examples
self._server.handle_request()
Expand All @@ -271,6 +273,7 @@ def _get_auth_response(self, result, auth_uri=None, timeout=None, state=None,

def close(self):
"""Either call this eventually; or use the entire class as context manager"""
self._closing = True
self._server.server_close()

def __enter__(self):
Expand Down

0 comments on commit 0deba2e

Please sign in to comment.