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

Store Edge instance in Environemnt record sync-ly #1595

Merged

Conversation

amartya4256
Copy link
Contributor

This PR contributes to storing the edge instance on creation in the webenvironment record synchronously for the sake of the consistency of the record. Since the record can be read by the cookie manager for several purposes on client calls, we need to make sure the record is in a consistent state, i.e. Linking the Edge instances (completely initialized or not) to their webview environment in the create method (synchronously).

In the bug #1592 the error occurs because we wait for the browser to initialize before it can be added to the record. It is safe to add it synchronously in the record since, all the further API calls of Edge browser is guarded by waitForFutureToFinish method.

contributes to #1592

Copy link
Contributor

github-actions bot commented Nov 13, 2024

Test Results

   483 files  ±0     483 suites  ±0   8m 42s ⏱️ -13s
 4 095 tests ±0   4 085 ✅ ±0   7 💤 ±0  3 ❌ ±0 
16 173 runs  ±0  16 080 ✅ ±0  90 💤 ±0  3 ❌ ±0 

For more details on these failures, see this check.

Results for commit 33188fe. ± Comparison against base commit 0e7f799.

♻️ This comment has been updated with latest results.

Copy link

@N1k145 N1k145 left a comment

Choose a reason for hiding this comment

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

I did test this with my actual product and it works now like before.
Thank you very much for the quick fix @amartya4256

@amartya4256
Copy link
Contributor Author

I did test this with my actual product and it works now like before. Thank you very much for the quick fix @amartya4256

Since we are adding it early, we also need to remove the instance if the initialization fails. I have amended the commit which includes the removal on failure as well.

This commit stores the edge instance on creation in the webenvironment
record synchronously for the sake of the consistency of the record.

contributes to eclipse-platform#1592
@HeikoKlare
Copy link
Contributor

Great, thank you for this fast fix!

As a short summary: this PR addresses a minor regression (#1592) introduced in this development cycle via #1378. The fix is simple and only affects Edge (an opt-in component), so there is low to no risk in this fix. The reporter of the regression @N1k145 confirmed that the proposed fix addresses the issue. In consequence (and in my opinion), this should go in RC1, which is why I kindly ask for PMC approval to merge this change @akurtakov @merks.

@akurtakov
Copy link
Member

As this change is in "opt-in" functionality, fixes regression and reporter verified it : PMC +1

@HeikoKlare
Copy link
Contributor

Thank you for the immediate approval! The failing tests are documented: #1564

@HeikoKlare HeikoKlare merged commit 1ee8024 into eclipse-platform:master Nov 13, 2024
11 of 14 checks passed
@fedejeanne fedejeanne added the pmc_approved Issues with PMC approval label Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pmc_approved Issues with PMC approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants