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

Support AF_INET6 address family #383

Merged
merged 5 commits into from
Jun 28, 2019
Merged

Support AF_INET6 address family #383

merged 5 commits into from
Jun 28, 2019

Conversation

Yolley
Copy link
Contributor

@Yolley Yolley commented Jun 26, 2019

Closes #313 .

As stated here https://docs.python.org/2/library/socket.html#socket.AF_INET6, "For AF_INET6 address family, a four-tuple (host, port, flowinfo, scopeid) is used", so there should be no possible problems with type conversion on row 12 of this PR. So I think, that there is no need for additional tests, already existing should be enough to cover this.

This PR should resolve the issue stated here encode#313 . 

As stated here https://docs.python.org/2/library/socket.html#socket.AF_INET6, "For AF_INET6 address family, a four-tuple (host, port, flowinfo, scopeid) is used", so there should be no possible problems with type conversion on row 12 of this PR. So I think, that there is no need for additional tests, already existing should be enough to cover this.
@tomchristie
Copy link
Member

Gotcha. Test cases currently failing, when looking up the “socket” extra info - not looked into it beyond that yet.

@Yolley
Copy link
Contributor Author

Yolley commented Jun 27, 2019

Gotcha. Test cases currently failing, when looking up the “socket” extra info - not looked into it beyond that yet.

It seems to me, that MockTransport in tests misses 'socket' attribute. Here is a documentation for BaseTransport (which is being mocked in tests, as i understand) https://docs.python.org/3/library/asyncio-protocol.html#asyncio.BaseTransport.get_extra_info - 'socket' is also an attribute among with sockname, peername and sslcontext . I guess, we need to fix MockTransport in tests. Should we do it in this pr or create another?

@tomchristie
Copy link
Member

Fixing it in this PR would be fine. However we ought to be a bit cautious about which items might or might not be present with get_extra_info. If we can fallback gracefully when particular items are not included then we should do that.

@Yolley
Copy link
Contributor Author

Yolley commented Jun 27, 2019

Fixing it in this PR would be fine. However we ought to be a bit cautious about which items might or might not be present with get_extra_info. If we can fallback gracefully when particular items are not included then we should do that.

Like, first try to get 'socket', then, if it is not present, just return peername/sockname?

@tomchristie
Copy link
Member

@Yolley Exactly, yes. I'd also suggest leaving the existing layout of the get_local_addr and get_remote_addr functions in place so that the change has a minimal-as-possible footprint.

def get_local_addr(transport):
    info = transport.get_extra_info("socket").  # Only these lines changed 
    if info is not None:                        # Only these lines changed 
        return info.getsockname()               # Only these lines changed 
    info = transport.get_extra_info("sockname")
    if info is not None and isinstance(info, (list, tuple)) and len(info) == 2:
        return (str(info[0]), int(info[1]))
    return None

@Yolley
Copy link
Contributor Author

Yolley commented Jun 28, 2019

@Yolley Exactly, yes. I'd also suggest leaving the existing layout of the get_local_addr and get_remote_addr functions in place so that the change has a minimal-as-possible footprint.

def get_local_addr(transport):
    info = transport.get_extra_info("socket").  # Only these lines changed 
    if info is not None:                        # Only these lines changed 
        return info.getsockname()               # Only these lines changed 
    info = transport.get_extra_info("sockname")
    if info is not None and isinstance(info, (list, tuple)) and len(info) == 2:
        return (str(info[0]), int(info[1]))
    return None

I made changes in this PR like you suggested, but we still need to make some changes in MockTransport, we need to change logic of method get_extra_info. In documentation https://docs.python.org/3/library/asyncio-protocol.html#asyncio.BaseTransport.get_extra_info it is stated, that get_extra_info should return default, if name is not present in transport. So, I guess we need (for example, here https://github.com/encode/uvicorn/blob/master/tests/protocols/test_http.py#L86) to do something like this

    def get_extra_info(self, key):
        return {
            "sockname": self.sockname,
            "peername": self.peername,
            "sslcontext": self.sslcontext,
        }.get(key)

@tomchristie
Copy link
Member

we still need to make some changes in MockTransport, we need to change logic of method get_extra_info

Yup, your suggestion there looks correct to me.

@Yolley
Copy link
Contributor Author

Yolley commented Jun 28, 2019

we still need to make some changes in MockTransport, we need to change logic of method get_extra_info

Yup, your suggestion there looks correct to me.

So, i fixed MockTransport, now it uses get on dictionary in get_extra_info. Also I added 2 new tests to test socket logic in utils. In travis I see, that it suggests some reformat for test_utils.py, but I don't understand, what needs to be reformatted exactly.

@tomchristie
Copy link
Member

Running ./scripts/lint will apply the correct formatting to the codebase.

@Yolley
Copy link
Contributor Author

Yolley commented Jun 28, 2019

Running ./scripts/lint will apply the correct formatting to the codebase.

Yep, it seems, that black follows pep8 line of length (which is questionable in 2019, but it is still an approved standart, so). Reformatted test_utils.py

@tomchristie tomchristie merged commit badcbc0 into encode:master Jun 28, 2019
@tomchristie
Copy link
Member

Released as 0.8.3. Good stuff! 👍

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.

Uvicorn can't get client's ip address, and port when listening to ipv6 address
2 participants