Skip to content

Refactor FXIOS-9778 Add Multipath TCP (MPTCP) support#21480

Merged
nbhasin2 merged 5 commits into
mozilla-mobile:mainfrom
Aperence:main
Oct 1, 2024
Merged

Refactor FXIOS-9778 Add Multipath TCP (MPTCP) support#21480
nbhasin2 merged 5 commits into
mozilla-mobile:mainfrom
Aperence:main

Conversation

@Aperence
Copy link
Copy Markdown
Contributor

@Aperence Aperence commented Aug 7, 2024

📜 Tickets

Github issue
Jira

💡 Description

MPTCP is a TCP extension allowing to improve network reliabilty by using mutiple network interface for the same TCP connection. Check https://www.mptcp.dev and https://developer.apple.com/documentation/foundation/urlsessionconfiguration/improving_network_reliability_using_multipath_tcp for details. Changes to this repository includes introducing new configurations (defaultMPTCP/ephemeralMPTCP), replacing uses of default/ephemeral by these new configurations, introducing a sharedMPTCP property on URLSession and modifying the makeURL function to enable the handover mode by default

📝 Checklist

You have to check all boxes before merging

  • Filled in the above information (tickets numbers and description of your work)
  • Updated the PR name to follow our PR naming guidelines
  • Wrote unit tests and/or ensured the tests suite is passing
  • When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
  • If needed, I updated documentation / comments for complex code and public methods
  • If needed, added a backport comment (example @Mergifyio backport release/v120)

@Aperence Aperence requested a review from a team as a code owner August 7, 2024 09:05
@Aperence Aperence requested a review from razvanlitianu August 7, 2024 09:05
@mattreaganmozilla
Copy link
Copy Markdown
Collaborator

Thanks for this PR @Aperence. Might be good to get some extra eyes on this (cc @nbhasin2).

@Aperence Aperence changed the title Refactor FXIOS-21470 - Add Mutipath TCP (MPTCP) support Refactor FXIOS-21470 - Add Multipath TCP (MPTCP) support Aug 14, 2024
Comment thread firefox-ios/Client/Extensions/URLSessionConfiguration+defaultMPTCP.swift Outdated
Comment thread firefox-ios/Client/Extensions/URLSessionConfiguration+defaultMPTCP.swift Outdated
Comment thread firefox-ios/Client/Extensions/URLSessionConfiguration+defaultMPTCP.swift Outdated
Copy link
Copy Markdown
Collaborator

@nbhasin2 nbhasin2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with minor nits

@nbhasin2 nbhasin2 requested review from mattreaganmozilla and removed request for razvanlitianu August 26, 2024 19:16
@nbhasin2
Copy link
Copy Markdown
Collaborator

image

@Aperence I see the following tests failing, would you mind looking into it and let us know if any support is required

@Aperence
Copy link
Copy Markdown
Contributor Author

Hello, I've fixed the issues with these tests normally

@mobiletest-ci-bot
Copy link
Copy Markdown

mobiletest-ci-bot commented Aug 27, 2024

Messages
📖 Project coverage: 31.56%
📖 Edited 18 files
📖 Created 2 files

Client.app: Coverage: 30.09

File Coverage
BrowserViewController.swift 5.07% ⚠️
TemporaryDocument.swift 0.0% ⚠️
URLSessionConfiguration+defaultMPTCP.swift 100.0%
PocketProvider.swift 86.21%
WallpaperNetworkModule.swift 100.0%
DownloadQueue.swift 50.15%
BreachAlertsClient.swift 0.0% ⚠️
ActionProviderBuilder.swift 0.0% ⚠️
SearchSuggestClient.swift 72.45%
ContileProvider.swift 90.91%
OpenPassBookHelper.swift 0.0% ⚠️

Generated by 🚫 Danger Swift against ea7dc48

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Aperence, somehow the warnings have increased. Let me re-run the build but I would suggest trying locally to check the warnings between main / your branch

Just posting in this file to have a chain of comments

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @nbhasin2,

Thank you for the review first.
After having checked the code, it seems like it was simply 2 immutable variables that were declared with a var keyword, instead of let. This should be fixed normally.

Also, I've seen that I forgot to add the Multipath entitlement, I've just added it now (if it is fine ?).

Have a nice day

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, totally missed this. Let me double check to see if this is valid.

@Aperence did you try it locally or simply added it where it needs to get added?

Thanks again

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I built locally the application to make sure that it doesn’t generate new warnings.

Thanks again for the follow-up !

@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented Sep 6, 2024

This pull request has conflicts when rebasing. Could you fix it @Aperence? 🙏

@Aperence
Copy link
Copy Markdown
Contributor Author

Aperence commented Sep 6, 2024

Rebased it normally !

@github-actions
Copy link
Copy Markdown
Contributor

This PR has been automatically marked as stale. Please leave any comment to keep this PR opened. It will be closed automatically if no further update occurs in the next 7 days. Thank you for your contributions!

@github-actions github-actions Bot added the stale Stalebot use this label to stale issues and PRs label Sep 21, 2024
@Aperence
Copy link
Copy Markdown
Contributor Author

Any news about this ?

@github-actions github-actions Bot removed the stale Stalebot use this label to stale issues and PRs label Sep 22, 2024
@nbhasin2 nbhasin2 requested a review from OrlaM September 24, 2024 19:22
Copy link
Copy Markdown
Collaborator

@OrlaM OrlaM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised this isn't just default URLSession behaviour and it's very strange that Apple require an entitlement for it 🤔
Great change either way, one minor nit otherwise LGTM

Comment thread firefox-ios/Client/Extensions/URLSessionConfiguration+defaultMPTCP.swift Outdated
@Aperence
Copy link
Copy Markdown
Contributor Author

Aperence commented Sep 25, 2024

Just applied the requested change. It probably isn't enabled by default because some time ago, some other options weren't (well) supported with MPTCP (from what I know), and they probably never reconsidered up to now to enable it by default.

@nbhasin2
Copy link
Copy Markdown
Collaborator

Running BR and if all looks well I can merge it in 🤞🏼

@nbhasin2 nbhasin2 requested a review from OrlaM September 27, 2024 01:51
@nbhasin2
Copy link
Copy Markdown
Collaborator

@Aperence might wanna rebase this one last time as you have approvals but there are certain number of warnings that are more on this PR compared to main.

Thanks and once green I can hit merge

MPTCP is a TCP extension allowing to improve network reliabilty by using mutiple network interface for the same TCP connection. Check https://www.mptcp.dev and
https://developer.apple.com/documentation/foundation/urlsessionconfiguration/improving_network_reliability_using_multipath_tcp for details.
Changes to this repository includes introducing new configurations (defaultMPTCP/ephemeralMPTCP), replacing uses of default/ephemeral
by these new configurations, introducing a sharedMPTCP property on URLSession and modifying the makeURL function to enable the handover
mode by default
Aperence and others added 4 commits September 30, 2024 08:04
Co-authored-by: Nishant Bhasin <nish.bhasin@gmail.com>
Changed 2 variabled from var to let, as they were never mutated
@Aperence
Copy link
Copy Markdown
Contributor Author

Aperence commented Sep 30, 2024

Just rebased it now, hope it will fix the warnings

@nbhasin2 nbhasin2 changed the title Refactor FXIOS-21470 - Add Multipath TCP (MPTCP) support Refactor FXIOS-9778 - Add Multipath TCP (MPTCP) support Oct 1, 2024
@nbhasin2 nbhasin2 changed the title Refactor FXIOS-9778 - Add Multipath TCP (MPTCP) support Refactor FXIOS-9778 Add Multipath TCP (MPTCP) support Oct 1, 2024
@nbhasin2 nbhasin2 merged commit bc92b4d into mozilla-mobile:main Oct 1, 2024
OrlaM added a commit that referenced this pull request Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants