Closed bavis-m closed 2 years ago
Thanks for the pull request! I've adapted these changes to better fit the style and code structure of the project, as well as taking the opportunity to refactor and deduplicate a bit of net code.
For future reference, there were a few blockers that would have prevented me from merging this as is, regardless of formatting and structure:
sockets_init()
cannot report errors, as all its return values implicitly cast to true
WSAStartup()
has no accompanying WSACleanup()
net_get_error_str_from()
uses a global variable, which isn't threadsafestrerror_s
requires __STDC_WANT_LIB_EXT1__
to be defined by the userstrerror_s
is not available on some systems (mine included)Thanks for cleaning that up.
Taking a bit of a further look, I think net_close
probably requires that close
to become closesocket
if #ifdef WINDOWS
, I missed that one. I'm guessing WSACleanup
probably doesn't go there either, it looks like socket operations can happen after net_close
(in net_cleanup
for sure, net_close
is called twice in a row). My personal opinion for my own codebases (take with a huge grain of salt haha) is that, since the socket system is needed (by definition) for the entire lifetime of the server, WSACleanup
probably isn't needed at all... we're already trusting Windows will cleanup socket resources (e.g. in a crash).
Thanks for the great project!
Citing Microsoft's documentation:
An application must call the WSACleanup function for every successful time the WSAStartup function is called. This means, for example, that if an application calls WSAStartup three times, it must call WSACleanup three times. The first two calls to WSACleanup do nothing except decrement an internal counter; the final WSACleanup call for the task does all necessary resource deallocation for the task.
As for not needing it, BeatUpServer needs to pass validation with Memcheck and AddressSanitizer, so all resources need to be cleaned up.