Skip to content

Fix: Ctrl+C results in handle_request failure #405

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

Closed
wants to merge 2 commits into from

Conversation

jiasli
Copy link
Contributor

@jiasli jiasli commented Sep 10, 2021

Refine #404

Symptom

_AuthCodeReceiver is used as a context manager:

with _AuthCodeReceiver(port=listen_port) as receiver:

When Ctrl+C is pressed, Python calls

def __exit__(self, exc_type, exc_val, exc_tb):
self.close()

then

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

This shuts down the server and causes

to return.

Because of the outer loop


,

is called again. However, as the server has been shut down, it fails with

Exception in thread Thread-1:
Traceback (most recent call last):
  File "C:\Users\user1\AppData\Local\Programs\Python\Python39\lib\threading.py", line 973, in _bootstrap_inner
    self.run()
  File "C:\Users\user1\AppData\Local\Programs\Python\Python39\lib\threading.py", line 910, in run
    self._target(*self._args, **self._kwargs)
  File "d:\cli-msal\microsoft-authentication-library-for-python\msal\oauth2cli\authcode.py", line 264, in _get_auth_response
    self._server.handle_request()
  File "C:\Users\user1\AppData\Local\Programs\Python\Python39\lib\socketserver.py", line 291, in handle_request
    selector.register(self, selectors.EVENT_READ)
  File "C:\Users\user1\AppData\Local\Programs\Python\Python39\lib\selectors.py", line 300, in register
    key = super().register(fileobj, events, data)
  File "C:\Users\user1\AppData\Local\Programs\Python\Python39\lib\selectors.py", line 239, in register
    key = SelectorKey(fileobj, self._fileobj_lookup(fileobj), events, data)
  File "C:\Users\user1\AppData\Local\Programs\Python\Python39\lib\selectors.py", line 226, in _fileobj_lookup
    return _fileobj_to_fd(fileobj)
  File "C:\Users\user1\AppData\Local\Programs\Python\Python39\lib\selectors.py", line 42, in _fileobj_to_fd
    raise ValueError("Invalid file descriptor: {}".format(fd))
ValueError: Invalid file descriptor: -1

Change

If a request is handled by msal.oauth2cli.authcode._AuthCodeHandler.do_GET, self.server.auth_response will always be set. Later on, if self._server.handle_request() returns leaving an empty self.server.auth_response, that means the server is shutdown before any request is handled, so we break the loop.

@jiasli
Copy link
Contributor Author

jiasli commented Sep 10, 2021

Azure CLI doesn't have this issue because Azure CLI doesn't close the server upon receiving Ctrl+C at all!

@jiasli
Copy link
Contributor Author

jiasli commented Sep 10, 2021

My initial implementation has a flaw: if some noises reach first, self.server.auth_response will be set, and self._server.handle_request() will be called again, triggering the failure. This can be reproed with curl:

  1. python -m azure.cli login --debug
  2. In another terminal, curl localhost:xxxxx
  3. Go back to the previous terminal, press Ctrl+C

However, when I visit localhost:xxxxx in Chrome, after pressing Ctrl+C, self._server.handle_request() never exits and the failure is not triggered. Meanwhile, localhost:xxxxx doesn't respond to curl anymore. Guess Chrome is holding the socket for some reason, preventing self._server.handle_request() from exiting. Once Chrome is closed, everything resumes to normal.

@rayluo
Copy link
Collaborator

rayluo commented Sep 15, 2021

Closing this, since we currently choose a boolean flag approach.

@rayluo rayluo closed this Sep 15, 2021
@jiasli jiasli deleted the handle_request branch September 20, 2021 02:51
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