-[HTTPServer start:] will no longer fail if it can't bind an IPv6 socket#114
Conversation
Codecov Report
@@ Coverage Diff @@
## master #114 +/- ##
=======================================
Coverage 77.89% 77.89%
=======================================
Files 39 39
Lines 2474 2474
Branches 124 124
=======================================
Hits 1927 1927
Misses 484 484
Partials 63 63Continue to review full report at Codecov.
|
StevenEWright
left a comment
There was a problem hiding this comment.
Should we consider IPv6-only as well as IPv4-only?
| return YES; | ||
| } | ||
|
|
||
| - (BOOL)hasIPv4Socket |
There was a problem hiding this comment.
For consistency:
- (BOOL)hasIPv4Socket {
| return ipv4socket != nil; | ||
| } | ||
|
|
||
| - (BOOL)hasIPv6Socket |
There was a problem hiding this comment.
For consistency:
- (BOOL)hasIPv6Socket {
| - (BOOL)start:(NSError **)error; | ||
| - (BOOL)stop; | ||
|
|
||
| - (BOOL)hasIPv4Socket; |
There was a problem hiding this comment.
Since start: always returns NO if it can't create an IPv4 socket, why is this necessary?
| - (BOOL)start:(NSError **)error; | ||
| - (BOOL)stop; | ||
|
|
||
| - (BOOL)hasIPv4Socket; |
There was a problem hiding this comment.
Why not something like:
@property(nonatomic, readonly) NSURL *preferredLoopbackURL;
There was a problem hiding this comment.
I don't know that there's much gain to splitting out part of startHTTPListener: into a separate property. The method is only a few lines as is.
There was a problem hiding this comment.
This comment was more to do with making life easy for consumers of this class - since the only reason consumers care about these values is in the context of determining which loopback address to use.
| } | ||
| return nil; | ||
| } else { | ||
| } else if ([_httpServ hasIPv4Socket]) { |
There was a problem hiding this comment.
The order of these clauses would cause the library to always use IPv4, since, with your changes, the library always must have an IPv4 socket, but only may have an IPv6 socket, this first test will always pass, leaving no chance for the second test.
There was a problem hiding this comment.
This was intentional, as I didn't want to change the behavior of the library dramatically without any discussion. Handling a potential IPv6-only case is easy enough to do, I'll add it even though it's unlikely to be hit any time soon.
There was a problem hiding this comment.
That would be very(!) much appreciated.
I do know Apple is concerned with IPv6-only networks (see here), so I think the feature may be more valuable than you're giving it credit for. IMHO, the lack of this support is a bug with the existing code.
|
|
|
This PR LGTM. @ksuther Thank you! Were you able to test it, or should I patch it in and try it? |
|
I tested it by making the IPv4 socket fail, but I didn't actually turn off IPv4 systemwide. |
|
Does this require anything else from me? |
|
@ksuther no. Thank you very much for this! We're just waiting for william to squash merge the PR, which he said he'd do. |
|
@ksuther, do you want to squash the commits into one? I can do this for you but then I'm added as the "committer", so thought I'd give you first option. |
…ket (it will only fail if can't bind an IPv4 and IPv6 socket) This prevents failures if someone is running on a system where IPv6 has been completely disabled. Added IPv6 loopback support to -[OIDRedirectHTTPHandler startHTTPListener:], but it currently isn't used since -[HTTPServer start:] will fail if it can't bind an iPv4 socket. It has been tested and can be used in the future if necessary. Future-proofing OIDLoopbackHTTPServer if IPv4 isn't available
|
I squashed them, I hope that's what you were expecting. |
|
Perfect, thanks. |
We had a user with IPv6 completely disabled (on Mac), which was causing
-[HTTPServer start:]to fail.Since only the IPv4 loopback address is used, I've made
start:succeed even if it can't bind an IPv6 socket.I also added the option of using the IPv6 loopback address (
::1) in the future if iPv6 should be preferred over IPv4.