Skip to content

Eliminate static shared this in std.socket #5813

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.

Sign up for GitHub

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
andralex wants to merge 4 commits into from
Closed

Eliminate static shared this in std.socket #5813

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6e8be26
Eliminate static shared this in std.socket
andralex Oct 26, 2017
89238d2
No more TLS
andralex Oct 27, 2017
824bd7d
One more typo on Windows
andralex Oct 27, 2017
f501da5
Make initialize nothrow
andralex Jun 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
171 changes: 108 additions & 63 deletions std/socket.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,56 +289,96 @@ bool wouldHaveBlocked() nothrow @nogc
assert(rec == -1 && wouldHaveBlocked());
}

// Returns true iff getnameinfo is supported on this platform.
version(Windows) private @property bool getnameinfoSupported()
Copy link
Member

@CyberShadow CyberShadow Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add getaddrinfoSupported and getnameinfoSupported and not just make getnameInfoPointer / getaddrinfoPointer lazily-initialized property functions?

Copy link
Member Author

@andralex andralex Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not new to this PR - old code handled the cases when the pointers were null and used workaround solutions.

So, old code:

  • try to fetch the pointers at startup
  • compare the pointers against null in code

New code:

  • define xxxSupported and xxx, both doing initialization on first call
  • use xxxSupported instead of comparing pointers to null

Copy link
Member

@CyberShadow CyberShadow Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but, why not just keep comparing pointers to null?

Copy link
Member Author

@andralex andralex Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because you need to call initialize before comparing them to null.

Copy link
Member

@CyberShadow CyberShadow Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, so, why not turn the function pointer variables into function pointer returning property functions, in the same way that getnameinfoSupported are in the current version, but returning the pointer directly instead of a bool?

Copy link
Member Author

@andralex andralex Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be an alternative, but it seems more roundabout. To make sure I understand:

@property auto our_getnameinfoPtr() {
    initialize;
    return &getNameInfoPtr;
}
... using code ...
if (our_getnameinfoPtr)
{
    ...
    our_getnameinfoPtr(... arguments ...);
}

Is this the idea? Mine was to not use pointers at all, but instead have the simple "if xxxSupported returns true, you may call xxx" convention. It seems yours could be used to avoid two calls to initialize by using local variables:

... using code ...
auto gni = our_getnameinfoPtr;
if (gni)
{
    ...
    gni(... arguments ...);
}

Copy link
Member

@CyberShadow CyberShadow Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be an alternative, but it seems more roundabout.

I don't see how that would be more roundabout. If anything, adding an additional pair of functions would better fit the description of "roundabout".

It seems yours could be used to avoid two calls to initialize by using local variables:

I don't see the need to avoid performing two calls. The compiler ought to inline the function, at which point it can trivially conclude that under the if branch, the pointer cannot possibly be null, and eliminate the second comparison.

Copy link
Member Author

@andralex andralex Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, yeah, two functions are better than four.

{
initialize;
return getnameinfoPointer !is null;
}
// Ditto (always supported on Posix).
version(Posix) private enum bool getnameinfoSupported = true;

private immutable
// Returns true iff getaddrinfo is supported on this platform.
version(Windows) private @property bool getaddrinfoSupported()
{
typeof(&getnameinfo) getnameinfoPointer;
typeof(&getaddrinfo) getaddrinfoPointer;
typeof(&freeaddrinfo) freeaddrinfoPointer;
initialize;
assert((getaddrinfoPointer is null) == (freeaddrinfoPointer is null));
return getaddrinfoPointer !is null;
}
// Ditto (always supported on Posix).
version(Posix) private enum bool getaddrinfoSupported = true;

shared static this() @system
version(Windows) private
{
version (Windows)
{
WSADATA wd;
import core.atomic;

// Winsock will still load if an older version is present.
// The version is just a request.
int val;
val = WSAStartup(0x2020, &wd);
if (val) // Request Winsock 2.2 for IPv6.
throw new SocketOSException("Unable to initialize socket library", val);
shared typeof(&getnameinfo) getnameinfoPointer;
shared typeof(&getaddrinfo) getaddrinfoPointer;
shared typeof(&freeaddrinfo) freeaddrinfoPointer;

// These functions may not be present on older Windows versions.
// See the comment in InternetAddress.toHostNameString() for details.
auto ws2Lib = GetModuleHandleA("ws2_32.dll");
if (ws2Lib)
{
getnameinfoPointer = cast(typeof(getnameinfoPointer))
GetProcAddress(ws2Lib, "getnameinfo");
getaddrinfoPointer = cast(typeof(getaddrinfoPointer))
GetProcAddress(ws2Lib, "getaddrinfo");
freeaddrinfoPointer = cast(typeof(freeaddrinfoPointer))
GetProcAddress(ws2Lib, "freeaddrinfo");
}
auto our_getnameinfo(A...)(A a) @system
{
initialize;
return getnameinfoPointer(a);
}
else version (Posix)

auto our_getaddrinfo(A...)(A a) @system
{
getnameinfoPointer = &getnameinfo;
getaddrinfoPointer = &getaddrinfo;
freeaddrinfoPointer = &freeaddrinfo;
initialize;
return getaddrinfoPointer(a);
}
}

auto our_freeaddrinfo(A...)(A a) @system
{
initialize;
if (freeaddrinfoPointer)
// May be null if WSAStartup() failed, just do nothing
return;
return freeaddrinfoPointer(a);
}

shared static ~this() @system nothrow @nogc
{
version (Windows)
void initialize() @trusted nothrow
Copy link
Member

@CyberShadow CyberShadow Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm reading this correctly, this function needs to be called in a lot more places now (every single bit of public code that eventually calls WSA functions, so that it calls WSAStartup), and I don't see this patch doing it.

Copy link
Member Author

@andralex andralex Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, will fix. Thanks.

{
WSACleanup();
static shared bool processInitialized;
if (atomicLoad(processInitialized)) return;

import std.concurrency : initOnce;

static bool perform()
{
// Winsock will still load if an older version is present.
// The version is just a request.
WSADATA wd;
if (!WSAStartup(0x2020, &wd)) // Request Winsock 2.2 for IPv6.
// No harm done, pointers to functions will be null (no supported functionality)
return true;

static extern(C) void cleanup() { WSACleanup(); }
Copy link
Member

@MartinNowak MartinNowak Oct 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually quite nice that dmd mangles nested C functions, no?

atexit(&cleanup);

// These functions may not be present on older Windows versions.
// See the comment in InternetAddress.toHostNameString() for details.+ ;
if (auto ws2Lib = GetModuleHandleA("ws2_32.dll"))
{
getnameinfoPointer = cast(typeof(getnameinfoPointer))
GetProcAddress(ws2Lib, "getnameinfo");
getaddrinfoPointer = cast(typeof(getaddrinfoPointer))
GetProcAddress(ws2Lib, "getaddrinfo");
freeaddrinfoPointer = cast(typeof(freeaddrinfoPointer))
GetProcAddress(ws2Lib, "freeaddrinfo");
}
return true;
}

initOnce!processInitialized(perform());
}
}
else version(Posix)
{
alias our_getnameinfo = getnameinfo;
alias our_getaddrinfo = getaddrinfo;
alias our_freeaddrinfo = freeaddrinfo;
}

/**
* The communication domain used to resolve an address.
Expand Down Expand Up @@ -963,16 +1003,16 @@ private AddressInfo[] getAddressInfoImpl(scope const(char)[] node, scope const(c
{
import std.array : appender;

if (getaddrinfoPointer && freeaddrinfoPointer)
if (getaddrinfoSupported)
{
addrinfo* ai_res;

int ret = getaddrinfoPointer(
int ret = our_getaddrinfo(
node.tempCString(),
service.tempCString(),
hints, &ai_res);
enforce(ret == 0, new SocketOSException("getaddrinfo error", ret, &formatGaiError));
scope(exit) freeaddrinfoPointer(ai_res);
scope(exit) our_freeaddrinfo(ai_res);

auto result = appender!(AddressInfo[])();

Expand All @@ -989,15 +1029,16 @@ private AddressInfo[] getAddressInfoImpl(scope const(char)[] node, scope const(c
return result.data;
}

throw new SocketFeatureException("Address info lookup is not available " ~
"on this system.");
version(Windows)
throw new SocketFeatureException("Address info lookup is not available " ~
"on this system.");
}


@safe unittest
{
softUnittest({
if (getaddrinfoPointer)
if (getaddrinfoSupported)
{
// Roundtrip DNS resolution
auto results = getAddressInfo("www.digitalmars.com");
Expand Down Expand Up @@ -1026,7 +1067,7 @@ private AddressInfo[] getAddressInfoImpl(scope const(char)[] node, scope const(c
}
});

if (getaddrinfoPointer)
if (getaddrinfoSupported)
{
auto results = getAddressInfo(null, "1234", AddressInfoFlags.PASSIVE,
SocketType.STREAM, ProtocolType.TCP, AddressFamily.INET);
Expand Down Expand Up @@ -1074,7 +1115,7 @@ private ushort serviceToPort(scope const(char)[] service)
*/
Address[] getAddress(scope const(char)[] hostname, scope const(char)[] service = null)
{
if (getaddrinfoPointer && freeaddrinfoPointer)
if (getaddrinfoSupported)
{
// use getAddressInfo
auto infos = getAddressInfo(hostname, service);
Expand All @@ -1091,7 +1132,7 @@ Address[] getAddress(scope const(char)[] hostname, scope const(char)[] service =
/// ditto
Address[] getAddress(scope const(char)[] hostname, ushort port)
{
if (getaddrinfoPointer && freeaddrinfoPointer)
if (getaddrinfoSupported)
return getAddress(hostname, to!string(port));
else
{
Expand All @@ -1109,13 +1150,13 @@ Address[] getAddress(scope const(char)[] hostname, ushort port)
}


@safe unittest
version(Windows) @safe unittest
{
softUnittest({
auto addresses = getAddress("63.105.9.61");
assert(addresses.length && addresses[0].toAddrString() == "63.105.9.61");

if (getaddrinfoPointer)
if (getaddrinfoSupported)
{
// test via gethostbyname
auto getaddrinfoPointerBackup = getaddrinfoPointer;
Expand Down Expand Up @@ -1168,7 +1209,7 @@ Address[] getAddress(scope const(char)[] hostname, ushort port)
*/
Address parseAddress(scope const(char)[] hostaddr, scope const(char)[] service = null)
{
if (getaddrinfoPointer && freeaddrinfoPointer)
if (getaddrinfoSupported)
return getAddressInfo(hostaddr, service, AddressInfoFlags.NUMERICHOST)[0].address;
else
return parseAddress(hostaddr, serviceToPort(service));
Expand All @@ -1177,7 +1218,7 @@ Address parseAddress(scope const(char)[] hostaddr, scope const(char)[] service =
/// ditto
Address parseAddress(scope const(char)[] hostaddr, ushort port)
{
if (getaddrinfoPointer && freeaddrinfoPointer)
if (getaddrinfoSupported)
return parseAddress(hostaddr, to!string(port));
else
{
Expand All @@ -1189,13 +1230,13 @@ Address parseAddress(scope const(char)[] hostaddr, ushort port)
}


@safe unittest
version(Windows) @safe unittest
{
softUnittest({
auto address = parseAddress("63.105.9.61");
assert(address.toAddrString() == "63.105.9.61");

if (getaddrinfoPointer)
if (getaddrinfoSupported)
{
// test via inet_addr
auto getaddrinfoPointerBackup = getaddrinfoPointer;
Expand Down Expand Up @@ -1286,10 +1327,10 @@ abstract class Address
// deprecated, albeit commonly-available method when getnameinfo()
// is not available.
// http://technet.microsoft.com/en-us/library/aa450403.aspx
if (getnameinfoPointer)
if (getnameinfoSupported)
{
auto buf = new char[NI_MAXHOST];
auto ret = getnameinfoPointer(
auto ret = our_getnameinfo(
name, nameLen,
buf.ptr, cast(uint) buf.length,
null, 0,
Expand All @@ -1309,18 +1350,20 @@ abstract class Address
return assumeUnique(buf[0 .. strlen(buf.ptr)]);
}

throw new SocketFeatureException((numeric ? "Host address" : "Host name") ~
" lookup for this address family is not available on this system.");
version(Windows)
throw new SocketFeatureException(
(numeric ? "Host address" : "Host name") ~
" lookup for this address family is not available on this system.");
}

// Common code for toPortString and toServiceNameString
private string toServiceString(bool numeric) @trusted const
{
// See toHostNameString() for details about getnameinfo().
if (getnameinfoPointer)
if (getnameinfoSupported)
{
auto buf = new char[NI_MAXSERV];
enforce(getnameinfoPointer(
enforce(our_getnameinfo(
name, nameLen,
null, 0,
buf.ptr, cast(uint) buf.length,
Expand All @@ -1330,8 +1373,10 @@ abstract class Address
return assumeUnique(buf[0 .. strlen(buf.ptr)]);
}

throw new SocketFeatureException((numeric ? "Port number" : "Service name") ~
" lookup for this address family is not available on this system.");
version(Windows)
throw new SocketFeatureException(
(numeric ? "Port number" : "Service name") ~
" lookup for this address family is not available on this system.");
}

/**
Expand Down Expand Up @@ -1613,7 +1658,7 @@ public:
// deprecated getHostByAddr() if it could not be found. See also:
// http://technet.microsoft.com/en-us/library/aa450403.aspx

if (getnameinfoPointer)
if (getnameinfoSupported)
return super.toHostNameString();
else
{
Expand Down Expand Up @@ -1689,15 +1734,15 @@ public:
assert(ia.toString() == "127.0.0.1:80");
});

softUnittest({
version(Windows) softUnittest({
// test reverse lookup
auto ih = new InternetHost;
if (ih.getHostByName("digitalmars.com"))
{
const ia = new InternetAddress(ih.addrList[0], 80);
assert(ia.toHostNameString() == "digitalmars.com");

if (getnameinfoPointer)
if (getnameinfoSupported)
{
// test reverse lookup, via gethostbyaddr
auto getnameinfoPointerBackup = getnameinfoPointer;
Expand All @@ -1709,13 +1754,13 @@ public:
}
});

version (SlowTests)
version(Windows) version (SlowTests)
softUnittest({
// test failing reverse lookup
const InternetAddress ia = new InternetAddress("127.114.111.120", 80);
assert(ia.toHostNameString() is null);

if (getnameinfoPointer)
if (getnameinfoSupported)
{
// test failing reverse lookup, via gethostbyaddr
auto getnameinfoPointerBackup = getnameinfoPointer;
Expand Down