Disable OpenSSL QtNetwork backend to avoid Windows crash #723
Conversation
WalkthroughThe pull request introduces modifications to the GitHub Actions workflow and the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
The Qt docs warn against deleting a QNetworkReply while the `finished` handler is running (https://doc.qt.io/qt-6/qnetworkreply.html). We're not explicitly deleting it here, just setting the Python reference to `None`, but that might be enough to cause the object to be destroyed, explaining the crash we're seeing on Windows. Instead we set the option on QNetworkAccessManager to auto-delete replies.
This is to work around a bug in PyInstaller that can cause the wrong DLLs to be loaded, leading to a crash.
jonathanperret
force-pushed
the
fix-windows-start-crash
branch
from
91cfdca to
6ea9698
Compare
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/main/python/main/ayab/version_checker.py (1)
Line range hint
1-99: Consider adding fallback mechanisms for version checkingGiven that this PR disables the OpenSSL backend, which could potentially affect network functionality, consider:
- Implementing a retry mechanism with exponential backoff for failed requests
- Adding an alternative version check method (e.g., local cache, different API endpoint) as fallback
- Making the version check more resilient to network backend changes
This would improve the robustness of the version checking system across different configurations.
.github/workflows/build-multi-os.yml (1)
271-286: LGTM! Well-documented solution to the OpenSSL backend issue.The implementation follows Qt's recommended approach for handling OpenSSL dependencies. The detailed comments explain the rationale and link to relevant documentation, making future maintenance easier.
Consider adding a comment with the PyInstaller issue link (pyinstaller/pyinstaller#8857) to help track when this workaround can be removed once PyInstaller fixes the underlying issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/build-multi-os.yml(1 hunks)src/main/python/main/ayab/version_checker.py(4 hunks)
🔇 Additional comments (5)
src/main/python/main/ayab/version_checker.py (4)
54-62: LGTM! Enhanced error diagnostics
The additional logging of HTTP status and response length will help diagnose network-related issues, particularly useful for tracking down connectivity problems.
76-84: LGTM! Improved code clarity
The restructured conditionals and additional logging enhance code readability and debugging capabilities.
99-99: LGTM! Proper resource cleanup
The cleanup logging is helpful, and the removal of explicit reply cleanup is correct since setAutoDeleteReplies(True) handles this automatically.
22-30: Verify compatibility with disabled OpenSSL backend
While the changes improve logging and memory management, we need to ensure this network code works when the OpenSSL QtNetwork backend is disabled.
.github/workflows/build-multi-os.yml (1)
271-286: Verify SSL functionality with native Windows backend.
While the solution elegantly resolves the crash by using Windows' native SSL implementation, it's important to verify that all SSL-related functionality works correctly with the native backend.
Run this script to identify SSL-dependent code that might be affected:
✅ Verification successful
Based on the search results, I can now generate a final response as we have sufficient information about the SSL usage in the codebase.
SSL functionality change appears safe with Windows native backend
The codebase's only SSL-dependent network functionality is in version_checker.py, which performs HTTPS requests to check for updates. The Windows native SSL backend (schannel) is a mature and well-tested component that fully supports modern HTTPS, making this change safe.
- The only Qt network classes used are
QNetworkAccessManagerandQNetworkRequestinversion_checker.py - No direct SSL socket manipulation or custom SSL configurations are present
- All HTTPS URLs found are for standard web requests that are fully supported by Windows' native SSL stack
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SSL/HTTPS related network calls
echo "Searching for SSL/HTTPS related code..."
rg -l 'https://' || true
rg -l 'QSslSocket|QSslConfiguration' || true
# Search for Qt network functionality that might use SSL
echo "Searching for Qt network functionality..."
ast-grep --pattern 'QNetworkAccessManager' || true
ast-grep --pattern 'QNetworkRequest' || true
Length of output: 2934
Problem
Fixes #720.
Proposed solution
With the help of @Adrienne200 I tracked the crash down to an incorrect OpenSSL DLL being loaded in the AYAB process: instead of the
libssl-3-x64.dlllibrary bundled with AYAB, the library (on one crashing computer) was being loaded fromC:\Program Files\HP\HP One Agent\libssl-3-x64.dll!The reason for the bundled library not being loaded is a PyInstaller bug that I reported: pyinstaller/pyinstaller#8857
We won't need to wait for the fix to PyInstaller though, because in this PR I opted to entirely remove the affected Qt plugin (the OpenSSL-backed QtNetwork backend), as suggested in the Qt documentation when OpenSSL-specific features are not needed: https://doc.qt.io/qt-6/ssl.html#considerations-while-packaging-your-application
Testing
I created a test release from this PR's head commit: https://github.com/jonathanperret/ayab-desktop/releases/tag/0.99.2-wincrash4
Summary by CodeRabbit
New Features
Bug Fixes
Chores