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

Closes #388: Run Rust FxA calls in single thread context #399

Merged
merged 3 commits into from Jul 10, 2018

Conversation

@carolkng
Copy link
Contributor

@carolkng carolkng commented Jul 10, 2018

Creates an FxaClient-specific single thread context, then moves all launch calls and runBlocking calls (for non-async methods calling into the Rust library) into that context.

r? @csadilek @thomcc

@pocmo pocmo requested a review from csadilek Jul 10, 2018
@codecov
Copy link

@codecov codecov bot commented Jul 10, 2018

Codecov Report

Merging #399 into master will increase coverage by 0.73%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #399      +/-   ##
============================================
+ Coverage     73.39%   74.12%   +0.73%     
  Complexity      738      738              
============================================
  Files           119      119              
  Lines          2935     2906      -29     
  Branches        411      398      -13     
============================================
  Hits           2154     2154              
+ Misses          568      539      -29     
  Partials        213      213
Impacted Files Coverage Δ Complexity Δ
...a/mozilla/components/service/fxa/FirefoxAccount.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
...main/java/mozilla/components/service/fxa/Config.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
.../java/mozilla/components/service/fxa/RustObject.kt 0% <0%> (ø) 0 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78ef679...66609be. Read the comment docs.

Copy link
Contributor

@csadilek csadilek left a comment

This looks good to me. We have confined all FxA methods to run on a single thread (both the sync and the async ones).

We need to figure out how to release this single thread when we don't need it anymore (to save memory). I've filed a follow-up issue for that: #402

@csadilek
Copy link
Contributor

@csadilek csadilek commented Jul 10, 2018

@pocmo are you ok with this as well? I didn't find an easy way to release the thread on timeout. I filed a follow-up to dig deeper.

@pocmo
pocmo approved these changes Jul 10, 2018
Copy link
Contributor

@pocmo pocmo left a comment

Yes, looks good! 👍

@carolkng
Copy link
Contributor Author

@carolkng carolkng commented Jul 10, 2018

From Slack discussion with Thom: instead of starting the new thread context and worrying about when to close it, the blocks inside the coroutines can be synchronized on the FxaClient instance.

@carolkng carolkng force-pushed the carolkng:thread-review branch from 9662b72 to d867e58 Jul 10, 2018
@csadilek
Copy link
Contributor

@csadilek csadilek commented Jul 10, 2018

@thomcc I think this does it. Want to take another look? We can refactor this later to introduce safeAsynRustCall and safeRustCall methods in RustObject. So, we don't have to do the launch / synchronized dance every time, but I think this is ready to go in now.

@thomcc
thomcc approved these changes Jul 10, 2018
Copy link
Contributor

@thomcc thomcc left a comment

Yeah seems good to me. I'd definitely prefer it if you reduced the duplication (since messing it up == memory unsafety), but if you'd rather do that in a follow up I can deal.

@carolkng carolkng force-pushed the carolkng:thread-review branch from d867e58 to 66609be Jul 10, 2018
@csadilek
Copy link
Contributor

@csadilek csadilek commented Jul 10, 2018

@carolkng fixed the code duplication by adding the safeAsync and safeSync calls now. Merging, as we have everything we want here ;).

@csadilek csadilek merged commit ac718ef into mozilla-mobile:master Jul 10, 2018
4 of 5 checks passed
4 of 5 checks passed
@codecov
codecov/patch 0% of diff hit (target 73.39%)
Details
Taskcluster (pull_request) TaskGroup: success
Details
@codecov
codecov/project 74.12% (+0.73%) compared to 78ef679
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sputnik Sputnik approves!
Details
bors bot pushed a commit that referenced this pull request Nov 14, 2019
4974: Add library for P2P communication and sample app r=jonalmeida a=espertus

Add library for P2P communication and sample app

This adds `lib-nearby` on top of the Google Play Nearby API.
The sample app `nearby-chat` uses it to create a chat application enabling
communication between 2 devices.

This is a better version of #4794 because it is a single commit.



5045: Bump rubyzip from 1.2.3 to 2.0.0 in /docs r=pocmo a=dependabot[bot]

Bumps [rubyzip](https://github.com/rubyzip/rubyzip) from 1.2.3 to 2.0.0.
<details>
<summary>Release notes</summary>

*Sourced from [rubyzip's releases](https://github.com/rubyzip/rubyzip/releases).*

> ## v2.0.0
> Security
> 
> - Default the `validate_entry_sizes` option to `true`, so that callers can trust an entry's reported size when using `extract` [#403](https://github-redirect.dependabot.com/rubyzip/rubyzip/pull/403)
>    - This option defaulted to `false` in 1.3.0 for backward compatibility, but it now defaults to `true`. If you are using an older version of ruby and can't yet upgrade to 2.x, you can still use 1.3.0 and set the option to `true`.
> 
> Tooling / Documentation
> 
> - Remove test files from the gem to avoid problems with antivirus detections on the test files [#405](https://github-redirect.dependabot.com/rubyzip/rubyzip/pull/405) / [#384](https://github-redirect.dependabot.com/rubyzip/rubyzip/issues/384)
> - Drop support for unsupported ruby versions [#406](https://github-redirect.dependabot.com/rubyzip/rubyzip/pull/406)
> 
> ## v1.3.0
> Security
> 
> - Add `validate_entry_sizes` option so that callers can trust an entry's reported size when using `extract` [#403](https://github-redirect.dependabot.com/rubyzip/rubyzip/pull/403)
>    - This option defaults to `false` for backward compatibility in this release, but you are strongly encouraged to set it to `true`. It will default to `true` in rubyzip 2.0.
> 
> New Feature
> 
> - Add `add_stored` method to simplify adding entries without compression [#366](https://github-redirect.dependabot.com/rubyzip/rubyzip/pull/366)
> 
> Tooling / Documentation
> 
> - Add more gem metadata links [#402](https://github-redirect.dependabot.com/rubyzip/rubyzip/pull/402)
> 
> ## v1.2.4
> - Do not rewrite zip files opened with `open_buffer` that have not changed [#360](https://github-redirect.dependabot.com/rubyzip/rubyzip/pull/360)
> 
> Tooling / Documentation
> 
> - Update `example_recursive.rb` in README [#397](https://github-redirect.dependabot.com/rubyzip/rubyzip/pull/397)
> - Hold CI at `trusty` for now, automatically pick the latest ruby patch version, use rbx-4 and hold jruby at 9.1 [#399](https://github-redirect.dependabot.com/rubyzip/rubyzip/pull/399)
</details>
<details>
<summary>Changelog</summary>

*Sourced from [rubyzip's changelog](https://github.com/rubyzip/rubyzip/blob/master/Changelog.md).*

> # 2.0.0 (2019-09-25)
> 
> Security
> 
> - Default the `validate_entry_sizes` option to `true`, so that callers can trust an entry's reported size when using `extract` [#403](https://github-redirect.dependabot.com/rubyzip/rubyzip/pull/403)
>    - This option defaulted to `false` in 1.3.0 for backward compatibility, but it now defaults to `true`. If you are using an older version of ruby and can't yet upgrade to 2.x, you can still use 1.3.0 and set the option to `true`.
> 
> Tooling / Documentation
> 
> - Remove test files from the gem to avoid problems with antivirus detections on the test files [#405](https://github-redirect.dependabot.com/rubyzip/rubyzip/pull/405) / [#384](https://github-redirect.dependabot.com/rubyzip/rubyzip/issues/384)
> - Drop support for unsupported ruby versions [#406](https://github-redirect.dependabot.com/rubyzip/rubyzip/pull/406)
> 
> # 1.3.0 (2019-09-25)
> 
> Security
> 
> - Add `validate_entry_sizes` option so that callers can trust an entry's reported size when using `extract` [#403](https://github-redirect.dependabot.com/rubyzip/rubyzip/pull/403)
>    - This option defaults to `false` for backward compatibility in this release, but you are strongly encouraged to set it to `true`. It will default to `true` in rubyzip 2.0.
> 
> New Feature
> 
> - Add `add_stored` method to simplify adding entries without compression [#366](https://github-redirect.dependabot.com/rubyzip/rubyzip/pull/366)
> 
> Tooling / Documentation
> 
> - Add more gem metadata links [#402](https://github-redirect.dependabot.com/rubyzip/rubyzip/pull/402)
> 
> # 1.2.4 (2019-09-06)
> 
> - Do not rewrite zip files opened with `open_buffer` that have not changed [#360](https://github-redirect.dependabot.com/rubyzip/rubyzip/pull/360)
> 
> Tooling / Documentation
> 
> - Update `example_recursive.rb` in README [#397](https://github-redirect.dependabot.com/rubyzip/rubyzip/pull/397)
> - Hold CI at `trusty` for now, automatically pick the latest ruby patch version, use rbx-4 and hold jruby at 9.1 [#399](https://github-redirect.dependabot.com/rubyzip/rubyzip/pull/399)
</details>
<details>
<summary>Commits</summary>

- [`2825898`](rubyzip/rubyzip@2825898) Merge pull request [#408](https://github-redirect.dependabot.com/rubyzip/rubyzip/issues/408) from rubyzip/v2-0-0
- [`cb407b1`](rubyzip/rubyzip@cb407b1) Bump version to 2.0.0
- [`e1d9af6`](rubyzip/rubyzip@e1d9af6) Merge pull request [#406](https://github-redirect.dependabot.com/rubyzip/rubyzip/issues/406) from rubyzip/bump-supported-ruby
- [`3641a96`](rubyzip/rubyzip@3641a96) Merge pull request [#405](https://github-redirect.dependabot.com/rubyzip/rubyzip/issues/405) from rubyzip/remove-test-files
- [`e79d9ea`](rubyzip/rubyzip@e79d9ea) Merge pull request [#407](https://github-redirect.dependabot.com/rubyzip/rubyzip/issues/407) from rubyzip/v1-3-0
- [`7c65e1e`](rubyzip/rubyzip@7c65e1e) Bump version to 1.3.0
- [`d65fe7b`](rubyzip/rubyzip@d65fe7b) Merge pull request [#403](https://github-redirect.dependabot.com/rubyzip/rubyzip/issues/403) from rubyzip/check-size
- [`35446f4`](rubyzip/rubyzip@35446f4) Drop old ruby and JDK versions from CI
- [`74d4bec`](rubyzip/rubyzip@74d4bec) Remove test files from gem
- [`97cb6ae`](rubyzip/rubyzip@97cb6ae) Warn when an entry size is invalid
- Additional commits viewable in [compare view](rubyzip/rubyzip@v1.2.3...v2.0.0)
</details>
<br />

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=rubyzip&package-manager=bundler&previous-version=1.2.3&new-version=2.0.0)](https://help.github.com/articles/configuring-automated-security-fixes)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)



Co-authored-by: Ellen Spertus <ellen.spertus@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants