-
Notifications
You must be signed in to change notification settings - Fork 547
Implementing Feature Switch for CFNetworkHandler and NSUrlSessionHandler #19003
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.
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
Conversation
@rolfbjarne I have made the changes you requested. Kindly review. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The changes don't compile:
did you try to build locally? |
This comment has been minimized.
This comment has been minimized.
@rolfbjarne I wasn't able to build it locally since I have a Windows system, so I just wrote the code. Is it possible for you to build locally and suggest the required changes? |
At that point I'll end up doing more work than if I were to do it myself... To work on this project you really need a Mac to be able to build and test your changes. |
So....what should I do now?? Do you have any ideas?? Really sorry for the inconvinience caused |
@Agnik7 Thank you for your contribution, we are a bit busy right now but we will try to get back to you as soon as possible, as @rolfbjarne mentioned you do need a Mac in order to build this project and do appreciate the intent so give us some time in order to move forward. |
Ok |
|
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
💻 [CI Build] Windows Integration Tests passed 💻✅ All Windows Integration Tests passed. Pipeline on Agent |
💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻✅ All tests on macOS M1 - Mac Big Sur (11.5) passed. Pipeline on Agent |
💻 [PR Build] Tests on macOS M1 - Mac Ventura (13.0) passed 💻✅ All tests on macOS M1 - Mac Ventura (13.0) passed. Pipeline on Agent |
📚 [PR Build] Artifacts 📚Packages generatedView packagesPipeline on Agent |
✅ API diff for current PR / commitLegacy Xamarin (No breaking changes)
NET (empty diffs)
✅ API diff vs stableLegacy Xamarin (No breaking changes).NET (No breaking changes)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
🚀 [CI Build] Test results 🚀Test results✅ All tests passed on VSTS: simulator tests. 🎉 All 232 tests passed 🎉 Tests counts✅ bcl: All 69 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have some numbers to see if the linker really is able to change the app size depending on this behavior.
This can probably be done by editing the size-comparison project here:
And check if there's any app size difference between:
- No change
- Setting
UseNativeHttpHandler=false
:
<PropertyGroup>
<UseNativeHttpHandler>false</UseNativeHttpHandler>
</PropertyGroup>
- Using NSUrlSessionHandler:
<PropertyGroup>
<MtouchHttpClientHandler>NSUrlSessionHandler</MtouchHttpClientHandler>
</PropertyGroup>
- Using CFNetworkHandler:
<PropertyGroup>
<MtouchHttpClientHandler>CFNetworkHandler</MtouchHttpClientHandler>
</PropertyGroup>
Then running make compare
in the tests/ subdirectory for each case should result in a comparison:
internal static RuntimeOptions? Read () | ||
{ | ||
#if NET | ||
var options = new RuntimeOptions (); | ||
if (Runtime.UseCFNetworkHandler) | ||
options.http_message_handler = CFNetworkHandlerValue; | ||
else if (Runtime.UseNSUrlSessionHandler) | ||
options.http_message_handler = NSUrlSessionHandlerValue; | ||
|
||
return options; | ||
#else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be simplified a lot more, by inlining this logic in the GetHttpMessageHandler method below:
internal static HttpMessageHandler GetHttpMessageHandler ()
{
#if NET
if (Runtime.UseCFNetworkHandler)
return new CFNetworkHandler ();
if (Runtime.UseNSUrlSessionHandler)
return new NSUrlSessionHandler ();
return new HttpClientHandler ();
#else
// existing logic
#endif
}
static bool useCFNetworkHandler = false; | ||
static bool useNSUrlSessionHandler = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assignment to the default value is unnecessary, except that the default is to use NSUrlSessionHandler:
static bool useCFNetworkHandler = false; | |
static bool useNSUrlSessionHandler = false; | |
static bool useCFNetworkHandler; | |
static bool useNSUrlSessionHandler = true; |
Hi @Agnik7. Due to inactivity, we will close this pull request in 7 days. |
2 similar comments
Hi @Agnik7. Due to inactivity, we will close this pull request in 7 days. |
Hi @Agnik7. Due to inactivity, we will close this pull request in 7 days. |
Hi @Agnik7. This pull request was closed due to inactivity. Please let us know if you'd like to reopen it. |
Continuation of #19003 This means the linker will remove either `CFNetworkHandler` or `NSUrlSessionHandler` depending on which is chosen at build time (or both, if neither is chosen). This will shrink app size a little bit, here are the types and members that are no longer included when using `NSUrlSessionHandler`: https://gist.github.com/rolfbjarne/102d77d5f105a7c98ac6be19de725dd5 A very basic app is ~55kb / 0.2% smaller: https://gist.github.com/rolfbjarne/a1cb6898aa560f0a891286be20b9bc3e Fixes #14298. --------- Co-authored-by: Agnik Bakshi <[email protected]> Co-authored-by: Dustin Wojciechowski <[email protected]> Co-authored-by: Michael Cummings <[email protected]> Co-authored-by: Alex Soto <[email protected]>
This PR is with respect to Issue #14298 .
Changes made:
UseCFNetworkHandler
andUseNSUrlSessionHandler
properties.RuntimeOptions.cs
to use the two new Runtime properties.