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.

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

Add a 2 second delay before shutting down the tunnel when encountering an error #1078

Merged
merged 9 commits into from
Nov 22, 2024

Conversation

samsymons
Copy link
Contributor

@samsymons samsymons commented Nov 15, 2024

Please review the release process for BrowserServicesKit here.

Required:

Task/Issue URL: https://app.asana.com/0/1207603085593419/1208772807738114/f
iOS PR: duckduckgo/iOS#3579
macOS PR: duckduckgo/macos-browser#3557
What kind of version bump will this require?: Patch

Description:

This PR adds a short delay when shutting down the tunnel.

Steps to test this PR:

  1. See client PRs

OS Testing:

  • iOS 14
  • iOS 15
  • iOS 16
  • macOS 10.15
  • macOS 11
  • macOS 12

Internal references:

Software Engineering Expectations
Technical Design Template

providerEvents.fire(.malformedErrorDetected(error))
try? await Task.sleep(interval: .seconds(2))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to wait for max 2s, or do we want to focus on reliability of sending? Asking cause we could use .fire call and wait for completion, but that could take some time.

Copy link
Contributor Author

@samsymons samsymons Nov 16, 2024

Choose a reason for hiding this comment

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

Focusing on reliability is a better path here, I'll adapt the EventMapper instance such that we can await the fire call and remove the sleep.

EDIT: Started working on this but the change isn't trivial, I'll ping for re-review once it's ready.

Using a name like `asyncFire` isn't ideal, but otherwise this function signature perfectly matches the function it's wrapping.
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 found an issue that's causing the tunnel to get blocked "Connecting..." forever. It seems like we had a bug that only surfaced now with these changes.

Additionally I'm hesitant to go forward with these changes, since I don't think pixels should block UI/UX.

Please see the comments.

Sources/Common/EventMapping.swift Outdated Show resolved Hide resolved
Sources/Common/EventMapping.swift Outdated Show resolved Hide resolved
Sources/Common/EventMapping.swift Outdated Show resolved Hide resolved
…g-error

* main:
  Bump github.com/duckduckgo/content-scope-scripts from 6.32.0 to 6.35.0 (#1080)
  Update VPN auth token check logic (#1082)
@samsymons
Copy link
Contributor Author

@diegoreymendez Sorry, you ran into some WIP changes that I didn't mark correctly on the PR. After our chat I've ended up reverting the WIP changes and gone back to the original approach, since I believe it will be enough to make a difference for now.

…g-error

* main:
  CSS: adding a new messageSecret properties for injected scripts (#1077)
  Bump github.com/duckduckgo/privacy-dashboard from 7.1.1 to 7.2.0 (#1079)
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 tested this and it works as described. The new error messages look neat too.

Nice job @samsymons

…g-error

* main:
  Bump C-S-S to 6.39.0 (#1089)
  Update Content Scope Scripts to 6.38.0 (#1087)
@samsymons samsymons merged commit deacf61 into main Nov 22, 2024
7 checks passed
@samsymons samsymons deleted the sam/delay-shutting-down-vpn-when-encountering-error branch November 22, 2024 02:38
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.

3 participants