Fix authentication mechanism #379
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As of now, during the authentication process, we, more or less, do the following: - Send an authentication request - Put a future that will be resolved after the authentication request completes to the pending connections that performs client-side authentication logic (ConnectionManager#on_auth) - Return the future The problem is that, through unlucky timing, we might perform the authentication logic before putting the future to the pending connections map. Such as, - Clients initiates get_or_connect in the main thread during startup - We send the authentication request in the main thread - Before we return the invocation future, and add a callback to it, Python schedules the reactor thread, we write the request to wire, and get back the response. - Python schedules the main thread again, it is now trying to add a callback to a completed future. Since it is completed, it executes the authentication logic immediately. One of the steps of the client side authentication logic is to remove the connection from the pending connections map. Since the future is not put into the map yet, nothing is removed. - We then add this future to the pending connections map, but since it is already completed, no one is going to remove it from the pending connections map. - During the shutdown, we see an unexpected entry in the pending connections map and fail. To solve this, we put a future directly into the pending connections and perform the logic afterward.
Contributor
What is a reactor thread |
Contributor
|
This is so interesting :) |
Contributor
yeah this is what I thought as well immediately after reading the problem |
srknzl
reviewed
Mar 26, 2021
| else: | ||
| e = response.exception() | ||
| # This will set the exception for the pending connection future | ||
| connection.close("Failed to authenticate connection", err) |
Contributor
There was a problem hiding this comment.
I guess this close calls cause self._authenticate(connection) call to raise exception. Is this right?
srknzl
approved these changes
Mar 26, 2021
Contributor
Author
|
Closing this in favor of #388 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As of now, during the authentication process, we, more or less, do the
following:
to the pending connections that performs client-side authentication logic
(ConnectionManager#on_auth)
The problem is that, through unlucky timing, we might perform the
authentication logic before putting the future to the pending connections
map. Such as,
Python schedules the reactor thread, we write the request to wire,
and get back the response.
a callback to a completed future. Since it is completed, it executes
the authentication logic immediately. One of the steps of the client
side authentication logic is to remove the connection from the pending
connections map. Since the future is not put into the map yet, nothing
is removed.
it is already completed, no one is going to remove it from the
pending connections map.
connections map and fail.
To solve this, we put a future directly into the pending connections
and perform the logic afterward.
Closes #378