HostResolver: bitte um Review

[mhat]

Hallo!

Ich hab's nicht so mit threads & syncing, deshalb hier mal meine kleine, einfache HostResolver-Klasse zum Review

class HostResolver
	{
		struct ThreadData
		{
			HostResolver* thisPtr;
			std::atomic<unsigned int>& threadCount;
			void* eventHandle;
			std::wstring name;
		};

		std::atomic<unsigned int> threadCount;
		void* eventHandle;
	public:
		HostResolver();
		~HostResolver();
		void ResolveHost(const std::wstring& hostName);
		void WaitAll();
	private:
		static unsigned int __stdcall StartResolveHostThread(void* param);
		void ResolveHostThread(const std::wstring& name);

		virtual void OnResolveHost(const std::wstring& hostName, const std::vector<std::wstring>& ips) = 0;
	};

HostResolver::HostResolver()
	{
		eventHandle = CreateEvent(0, true, false, 0);
	}

	HostResolver::~HostResolver()
	{
		CloseHandle(eventHandle);
	}

	void HostResolver::ResolveHost(const std::wstring& name)
	{
		ResetEvent(eventHandle);
		++threadCount;

		_beginthreadex(0, 0, StartResolveHostThread, new ThreadData { this, threadCount, eventHandle, name }, 0, 0);
	}

	void HostResolver::WaitAll()
	{
		WaitForSingleObject(eventHandle, INFINITE);
	}

	unsigned int __stdcall HostResolver::StartResolveHostThread(void* param)
	{
		auto threadData = reinterpret_cast<ThreadData*>(param);

		static_cast<HostResolver*>(threadData->thisPtr)->ResolveHostThread(threadData->name);

		if(!--threadData->threadCount)
			SetEvent(threadData->eventHandle); // Hmm...

		delete threadData;

		return 0;
	}

	void HostResolver::ResolveHostThread(const std::wstring& name)
	{
		addrinfoW* addrInfo;
		addrinfoW hints = {};

		hints.ai_family = AF_INET;

		std::vector<std::wstring> ips;

		if(!GetAddrInfoW(name.c_str(), 0, 0, &addrInfo))
		{
			auto current = addrInfo;

			while(current)
			{
				wchar_t buffer[INET_ADDRSTRLEN];
				unsigned long bufferLength = sizeof(buffer) / sizeof(buffer[0]);

				if(!WSAAddressToString(addrInfo->ai_addr, addrInfo->ai_addrlen, 0, buffer, &bufferLength))
					ips.push_back(buffer);

				current = current->ai_next;
			}

			FreeAddrInfoW(addrInfo);
		}

		OnResolveHost(name, ips);
	}

Ich mache mir Sorgen bei "SetEvent(threadData->eventHandle);" in Zeile 55. Könnte es sein, dass SetEvent() intern noch auf eventHandle zugreift, während WaitForSingleObject() schon zurückgekehrt ist? Dann wäre nämlich sowas wie

hostResolver->WaitAll();
delete hostResolver;

gefährlich. Es crasht zwar nicht, aber man weiß ja nie...
Wie würdet ihr es lösen, dass man das Objekt löschen können soll, während noch Threads laufen?
(Sobald ich WaitAll() + delete aufrufen will, stelle ich natürlich sicher, dass von nirgends mehr neue ResolveHost() aufgerufen werden. WaitAll() in den Destruktor geht nicht, dann gibt's pure virtual function calls.)

Danke!

[mhat]

Ups, zwei Sachen:

Zeile 40 (CloseHandle vergessen):
CloseHandle(reinterpret_cast<void*>(_beginthreadex(0, 0, StartResolveHostThread, new ThreadData { this, threadCount, eventHandle, name }, 0, 0)));

Zeile 52 (static_cast unnötig):
threadData->thisPtr->ResolveHostThread(threadData->name);

[mhat]

Ohh, noch was:
Das event sollte natürlich initial gesetzt sein... also CreateEvent(0, true, true, 0);

[dwadwadfwaqfwaf]

CloseHandle(reinterpret_cast<void*>(_beginthreadex(0, 0, StartResolveHostThread, new ThreadData { this, threadCount, eventHandle, name }, 0, 0)));

Ist das dein ernst?

[mhat]

Ja, warum?

[Lowqbit]

Ich glaube bei dir...

[theta]

Hier mal einige Punkte, nicht vollständig...

a) Besser eine Möglichkeit für ein Callback in den HostResolver reingeben (Interface/Functionpointer o.ä.). So werden Abhängigkeiten aufgelöst/verringert und es muss nicht von HostResolver abgeleitet werden.

b) Der Benutzer der Klasse muss wissen, dass das Callback auf einem anderen Thread aufgerufen wird, als er sein HostResolver Objekt erzeugt hat. Es gibt Möglichkeiten um die Aufrufe wieder auf dem Thread auszuführen, auf welchem das HostResolver Objekt erzeugt wurde (Windows Messages, APC, Completion Ports...). So müsste sich der Benutzer der Klasse nicht um die Synchronisierung kümmern, um die Callbacks vom HostResolver zu behandeln.

c) Aktuell besteht die Möglichkeit, dass ein HostResolver Objekt zerstört wird, obwohl noch Threads am laufen sind (das hast du ja selbst schon erkannt). Das führt zu Problemen, denn die Thread-Funktionen greifen auf Membervariablen zu. Mit WaitAll() kann der Benutzer der Klasse das zwar verhindern, aber es ist ihm überlassen ob er das tut oder nicht. Besser wäre es im Destruktor noch zu warten, falls nötig, bevor mit zerstören weitergemacht wird. Zusammen mit a) sollte das keine Probleme mehr verursachen.

d) ResetEvent/SetEvent/WaitForSingleObject... sind Thread-Safe (denn sie dienen ja gerade der Synchronisierung), d.h. Zeile 55 (im initialen Post) ist m.M.n. in Ordnung.

e) Warum nicht die C++11 Werkzeuge für Threading/Synchronisierug benutzen? Oder gar Boost? Oder noch besser, den Resolver von Boost.Asio?

Edit: Link angepasst

[mhat]

Danke!

Zu c):

mhat schrieb:

WaitAll() in den Destruktor geht nicht, dann gibt's pure virtual function calls.

Zu e):
Bei Threads/Syncing habe ich mir irgendwie angewöhnt, direkt WinAPI zu verwenden. Werde mir aber wohl bald mal die neuen Funktionen anschauen.. Boost ist mir zu dick, brauche ich nicht..

[theta]

theta schrieb:

c) ...Besser wäre es im Destruktor noch zu warten, falls nötig, bevor mit zerstören weitergemacht wird. Zusammen mit a) sollte das keine Probleme mehr verursachen.

keine Probleme bezog sich da genau auf ...pure virtual function calls.