Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update latency & tunnel failure monitors implementation #613

Merged
merged 13 commits into from
Dec 22, 2023
Merged

Update latency & tunnel failure monitors implementation #613

merged 13 commits into from
Dec 22, 2023

Conversation

quanganhdo
Copy link
Member

@quanganhdo quanganhdo commented Dec 22, 2023 •
edited
Loading

Required:

Task/Issue URL: https://app.asana.com/0/0/1206209663744590/f and https://app.asana.com/0/0/1206212872939262/f
iOS PR: duckduckgo/iOS#2290
macOS PR: duckduckgo/macos-browser#2005
What kind of version bump will this require?: Patch

Description:

Updates the monitor implementation to use Task to address situations when timer is launched multiple times. This affects macOS where it may have caused pixel anomalies due.

OS Testing:

  • iOS 14
  • iOS 15
  • iOS 16
  • macOS 10.15
  • macOS 11
  • macOS 12
Internal references:

Software Engineering Expectations
Technical Design Template

@diegoreymendez
Copy link
Contributor

@quanganhdo - I'm starting the review for this PR. Could I kindly ask to always include a description of the changes and a list of manual tests, ideally with 100% coverage of the changes? Thanks!

diegoreymendez
diegoreymendez requested changes Dec 22, 2023
View reviewed changes
Copy link
Contributor

@diegoreymendez diegoreymendez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I currently can't test the macOS build as it's failing. Will check again when it's good. Added some initial feedback.

Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift Outdated Show resolved Hide resolved
@diegoreymendez diegoreymendez self-requested a review December 22, 2023 20:14
@diegoreymendez
Copy link
Contributor

This seems to work for me. That said I'm getting a lot of these:

⚫️ Ping error: The operation couldn’t be completed. (NetworkProtection.Pinger.PingError error 2.)

Any idea why that'd be the case?

@quanganhdo quanganhdo marked this pull request as ready for review December 22, 2023 21:41
diegoreymendez
diegoreymendez approved these changes Dec 22, 2023
View reviewed changes
Copy link
Contributor

@diegoreymendez diegoreymendez 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 approving, but please take a moment to look at my last comment since there's some cleanup to do before merging.

Sources/NetworkProtection/Diagnostics/NetworkProtectionLatencyMonitor.swift Outdated Show resolved Hide resolved
@quanganhdo quanganhdo merged commit 18043cb into main Dec 22, 2023
5 checks passed
@quanganhdo quanganhdo deleted the anh/multiple-timer-fix branch December 22, 2023 23:10
samsymons added a commit that referenced this pull request Dec 24, 2023
@samsymons
Merge branch 'main' into sam/add-netp-subscription-auth-support
8dd2ee0 
* main:
  Update latency & tunnel failure monitors implementation (#613)
  Prevents VPNSettings from reporting fake changes (#614)
  Update Link Tracking Protection to preserve headers (#600)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diegoreymendez diegoreymendez diegoreymendez approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@quanganhdo @diegoreymendez