Windows Support #1259
Merged
Windows Support #1259
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github test runners, which was showing up as memory leaks when using force-libevent.
Client Transport::onReady
EmEvent leak in case of DEBUG and libevent on. In http_server_test, for every test case encapsulating the body of each test to guarantee that local (stack) variables go out of scope before the google AddressSanitizer has a chance to test for memory leaks.
covers the whole of the test (not just the interior/"thread"-variable part)
check for EmEvent leak to every TEST.
In streaming_test, check for no EmEvent leaks solely in the final test
("ClientDisconnect")
define in pist_syslog.h) Tracking EmEvents in streaming_test
to a shared_ptr. Previously, the reference returned by getPeer was to a shared_ptr stored in the map peers_ - so if the entry was removed from peers_, or peers_ went out of scope, the reference could refer to a shared_ptr that no longer existed. Rather than closing a peer's Fd directly in transport, now call peer->closeFd(), which not only closes the Fd but allows peer to set it's fd_ to empty, helping ensure that the Fd is closed exactly once.
missing close was seen in certain environments (though not all environments) for streaming_test.ClientDisconnect. It reproduced reliably in github workflow using current linux.yaml and os: debian:stable; compiler: gcc; sanitizer: address; tls: true; flibev: true
Put back the exlcuded tests in streaming_test
for a libevent callback function (the callback function will result in the EmEvent being placed on ready_). Change linux.yaml to run solely http_server_test for the timebeing
include/... directories. No other changes.
the whole of shutdown. Otherwise we can have a two-mutex deadlock between queuesLock and handling_mutex during shutdown. See code comment for more details. Move my_basename_r into pist_syslog.h so it can be used by other code beyond eventmeth.cc. Moved GUARD_AND_DBG_LOG into pist_check.h, so it can be used by other code beyond eventmeth.cc.
Created a new linuxflibev.yaml which takes care of running the libevent builds and test cases
avoid any possible confusion with the regular linux tests.
so that it returns with interest_mutex_ locked; the caller (in os.cc) is then required to call unlockInterestMutexIfLocked() thereafter. This is used to prevent another thread closing (and invalidating) an Fd that has been returned to os.cc's Epoll::poll before Epoll::poll can finish processing the returned Fd. fix: In getReadyEmEvents, adding a short, local lock of ready_mutex_ to protect the ready_ set as we get and remove it's contents. docs: Added notes on other non-libevent-specific fixes to "macOS Implementation.txt" style: Corrected comment typo in client.cc
If this option is set to "true", then all logged messages will be sent to stdout in addition to the log file.
We are now calling lockInterestMutex *before* we grab the contents of ready_ in getReadyEmEventsHelper. That way, since the interest_mutex_ is required for closing an Fd, there is no risk of an Fd closing between ready_ being grabbed (ready_ could contain the Fd_) and lockInterestMutex being called. Updated some debug output in os.cc.
kiplingw
added
help wanted
portability
fix in progress
dependencies
winscripts/gccsetup.ps1 + winscripts/msvcsetup.ps1 prefix the PowerShell prompt with "GCC " or "MVS " respectively, if not already so prefixed. In checking the existing prompt for GCC/MVS, the powershell code was throwing an exception if the existing prompt was shorter than 3 chars. Now coping correctly with that case.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1259 +/- ##
==========================================
- Coverage 77.93% 75.10% -2.83%
==========================================
Files 57 58 +1
Lines 7914 8943 +1029
==========================================
+ Hits 6168 6717 +549
- Misses 1746 2226 +480 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
We wanted to fix this for Windows, and just fixed it for the other OS
at the same time.
windows.yaml
linux.yaml
linuxflibev.yaml
macos.yaml
- Add brotli and zstd to dependencies installed by the yaml file
(In windows, this is done in commonsetup.ps1, which is invoked by
gccsetup.ps1 and msvcsetup.ps1, which in turn are invoked by
windows.yaml)
(In linux.yaml, added for both Debian and RedHat dependency
install)
- Enable brotli and zstd encoding in "meson setup..." line (This
also ensures that the brotli and zstd tests are built in
http_server_test.cc)
commonsetup.ps1 (Windows only - used in windows.yaml, see above)
- Install brotli and zstd with vcpkg if not already present
Building on macOS.txt
Building on Windows.txt
Building on BSD - FreeBSD, OpenBSD and NetBSD.txt
- Updated to reflect brotli and zstd dependencies
Convenience scripts:
winscripts/mesbuild.ps1
winscripts/mesbuilddebug.ps1
bldscripts/mesbuild.sh
bldscripts/mesbuilddebug.sh
bldscripts/mesbuildflibev.sh
bldscripts/mesbuildflibevdebug.sh
- Updated so deflate, brotli and zstd meson setup options are all
defined to true for "meson setup ...".
http_server_test
- When filling with random data for deflate, zstd, or brotli tests,
use a std::vector<unsigned short>, not a std:vector of
bytes. Support for the vector of bytes is not actually required by
the standard, and causes an error on MSVC/Windows.
- Add a check to the return code of ZSTD_getFrameContentSize and
adjust appropriately if it is an error (previously, was treating
the error code as though it was a length).
- Pass the parameters to ZSTD_decompress in the correct order
- Revert some indentation to the existing indentation used in master
branch
Also:
bldscripts/clean.sh: Update to find correct build dir(s)
linux.yaml linuxflibev.yaml - For Debian builds, install libbrotli-dev and libzstd-dev - For RedHat builds, install libbrotli and libzstd Building on BSD - FreeBSD, OpenBSD and NetBSD.txt README.md - Updated to reflect the new libxxxx dependencies Also, http_server_test.cc: - Used pointer arithmetic rather than &(*std::end()) to get pointer to immediately-after the end of an array. Some versions of MSVC object to "&(*std::end())".
For brotli, we install the library brotli-devel There doesn't seem to be a working preconfigured zstd package for RedHat ubi8 or ubi9. We skip linking and testing zstd compression for RedHat builds.
In particular, passing the correct buffer to ZSTD_getFrameContentSize.
Following review of the windows PR pistacheio#1259 Include compression features - ZSTD, Brotli and Deflate all included in github runner tests Similarly included them in the mesbuild* convenience scripts client.cc: 715: Make dynamic receive buffer maximum smaller Use std::memcpy not mempy Don't use NULL, prefer nullptr thoughout net.cc: 346 tmp2 - set it to NULL check strtol ret code check tmp2 before dereferencing it winornix.h and throughout: Use macros for the se_err[256+16] strerror_r buffer stuff Use MAXPATHLEN for size for strerror_r buffer pist_ifaddrs.cc:120 and throughout: When it doesn't matter, use prefix ++ or --, not postfix (Changed this in my own code, mostly left it alone in other code) pist_ifaddrs.cc 218: declare name_len const 219, 234: remove C-casts pist_resource.cc: Remove C casts pist_strerror_r.cc 37: Put ::strcmp. Same for 49, 51, 53 pist_syslog.cc: 344 Make get_val_res const Also 371 Also 582, 1100, 1126 ps_basename.cc 104: Remove C-cast ps_sendfile.cc 75, 84: Remove C-cast ps_strl.cc 54, 58: set as const reactor.cc 40: lLoggedSetThreadDescriptionFail should be static. Also 47-49. 721-724: Make const transport.cc 1113: const_cast - made it mutable instead src/meson.build 111: Include windres in the comment pkgconfig - check pistachelog.dll is included. Looks like it should work. Added comment in meson.build. 256: Add comment on what warning 4102 is listener.cc: 975: Windows file handle not _always_ a multiple of 4. Add comment. installman.ps1: Add a summary comment at the top async_test.cc 296: First two parms should be cbegin, cend tests/http_server_test.cc 577: Replace "= 0" with "= nullptr" tests/listener_test.cc: Call strerror_r to get the "address in use" string to compare to 405: Don't Use (void). (switched to std::ignore) In scripts, move helpers into a "helpers" subdir Use location of script to determine location of helpers (Don't hardcode winscripts/helpers or bldscripts/helpers).
include/pistache/winornix.h: Previously, was using PST_MAXPATHLEN without including the necessary header file. Corrected. bldscripts/helpers/mesflibevsetdirvars.sh: Added a missing variable definition in the script
kiplingw
approved these changes
Nov 15, 2024
Also: In src/meson.build: Provide vs_module_defs parameter to meson "libpistache = library(..." command for Visual Studio case.
Newer headers (like pist_strerror_r.h) added to install_headers in /include/pistache/meson.build.
winornix.h, pist_strerror_r.h, pist_strerror_r.cc: Wrap strerror_r for Apple GCC case (Apple GCC defaults to XSI strerror_r, which is not the strerror_r we want). Fixes compile error for GCC on macOS. Also: winornix.h: we wrap errno.h with pst_errno.h in all non-Linux cases. Fixes a compile issue on openBSD.
kiplingw
approved these changes
Nov 21, 2024
Tachi107
reviewed
Nov 22, 2024
There was a problem hiding this comment.
Hi Duncan, thanks for your incredible work. Windows support is something many people wanted and this patch set will make them quite happy. I don't know how many people will actually run Pistache on a production environment with Windows, but still, portability is a worthwhile goal in itself, especially when it comes to supporting non-Unix systems. Unix made lot of things right, but it's not perfect (my bachelor's thesis is partly about this, in fact).
With that being said, I'm not going to review this. There are more then ten thousand changed lines, spanning over 300 commits, and it is hence impossible for anyone to review such work in one's free time. I would have preferred small, organized changes sent over time via multiple pull requests.
Kip, I leave this to you :)
Thanks again for your incredible work!
|
Hi @Tachi107 - Thank you for your consideration :-) Yes, indeed, this PR is essentially a large number of tiny changes to address syntactical and API matters, combined with some lumps of PowerShell script to configure the dev environment. Should you have additional comments related to the changes in future, I will of course be more than happy to take a look. Meanwhile, all the best! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses: Issue #6, #649, #869, #538, #1128.
The changes for Windows support fall into 5 main buckets:
Taking each in turn:
Build and environment
The meson.build files are adjusted to link Windows libraries; to compile and install Pistache's Windows logging methods (uses custom_target); and to export symbols from the DLL (including dump2def subproject).
Environment configuration: We presume development will be carried out via a PowerShell terminal (local or remote). The files winscripts/msvcsetup.ps1 and winscripts/gccsetup.ps1 are provided to configure development with Visual Studio or GCC respectively. The file Building on Windows.txt gives Windows-specific build instructions.
windows.yaml: Leverages msvcsetup.ps1 and gccsetup.ps1 to set up the build environment and install dependencies. Otherwise works similarly to other Pistache runners (linux.yaml etc.).
Windows logging support
We log using the Windows-preferred "EWT" methodology. The meson.build methods provide for the compilation and installation of the necessary logging manifest etc. At the level of the Pistache code, the
PS_LOG_xxxxmethods are used, just as per Linux, macOS etc.pist_syslog.cc: In addition to invoking the Windows logging functions, it also uses the Windows API to monitor the registry property HKCU:\Software\pistacheio\pistache\psLogToStdoutAsWell; by setting this property to 1 (or back to 0), the user can turn on (or off) verbose logging to stdout.
Syntactic and API difference support
There are differences between Linux and Windows in type and constant definitions, API call definitions, and C++ syntax. This causes a lot of small changes across the Pistache code base.
The header file winornix.h defines pre-processor macros and header-file names. By using these definitions, in most cases in the code base we avoid patterns such as:
As an example, there is no
ssize_ttype in Windows. We definePST_SSIZE_Tin winornix.h, which works in both Windows and *nix code, and reduce tossize_tin the *nix case.In a number of cases, we have provided wrappers (or occasionally larger implementations) for Windows so that we can have Linux-like function interfaces for use in the broader Pistache code base. These "Linux-like functions in Windows" are usually fronted with a preproccessor macro to abstract the Windows vs. *nix differences. As an example, the macro
PST_SOCK_BINDis used the same way in Windows and *nix; in Windows it invokes our functionpist_sock_bindand in the *nix case it reduces to::bind. Files that provide these kind of wrapper include:Event-handling support in Windows
Most of the differences between event handling in Windows vs. *nix are already abstracted away by the previously-implemented libevent support. The major remaining item is the lack of edge-triggered events in Windows; Windows is level-triggered only. Code in eventmeth.cc emulates edge-triggered behaviour sufficiently for Pistache, essentially by deactivating a activated event, and then, if appropriate, adding it back to the event polling loop ready for the next activation.
Misc/Other Items of Note
C-style casts. Having introduced additional C-style casts during the Windows-support implementation, we then went through to replace them with C++-style casts. It turned out to be easier to do that replacement for all C-style casts, whether newly introduced or not, so that's what we did.
client.cc: Reworked Transport::handleIncoming; its logic around receiving in multiple pieces (cf. use of totalBytes) didn't work before. Windows seems more prone to using multiple receive events to receive a single incoming datagram.
eventmeth.cc
reactor.cc
transport.cc - improvements when using TCP_NODELAY, which Windows requires.
listener.cc - use an atomic counter, not the file descriptor number, to generate an index into the array of handlers for a new connection. File descriptor numbers don't have the nice "increment by one each time" pattern in Windows that they do in Linux.
dump2def - subproject utility to assist in generating symbols for DLL export. This is needed becasue, by default, Windows DLLs do not export their symbols, unlike in Linux.
http_server_test.cc