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

[SM-1413] Temporary PR to Showcase 'hyper' Error #955

Closed
wants to merge 1 commit into from

Conversation

coltonhurst
Copy link
Member

๐ŸŽŸ๏ธ Tracking

https://bitwarden.atlassian.net/browse/SM-1413

๐Ÿ“” Objective

๐Ÿšจ Do Not Merge This PR ๐Ÿšจ

The goal of this PR is to allow a developer to easily reproduce a potential bug with hyper that we are running into.

Description

When running concurrent requests using the same client from the Go SDK binding, execution stops for at least one of the threads at runtime with the following error: runtime dropped the dispatch task.

Replication Steps

  • cargo build from root
  • npm run schemas from root
  • Move the built sdk/target/debug/libbitwarden_c.a to the sdk/languages/go/internal/cinterface/lib/{arch} folder, where {arch} is your CPU architecture.
    • For Mac arm CPU's: darwin-arm64
    • Other examples of potential folder names can be found here
  • Update the following variables for your environment in the sdk/languages/go/example/example.go file:
    • OrganizationId
    • AccessToken
    • statePath
  • From sdk/languages/go/example, run go build
  • From sdk/languages/go/example, run ./example > go-log.txt 2>&1

Open the go-log.txt file and you should find something similar to the following (certain data redacted and replaced with x):

[2024-08-13T23:04:01Z DEBUG reqwest::connect] starting new connection: http://localhost:4000/
SecretWithProjectsListResponseModel { object: Some("SecretsWithProjectsList"), secrets: Some([SecretsWithProjectsInnerSecret { id: Some(x), organization_id: Some(x), key: Some("x"), creation_date: Some("2024-08-01T20:09:48.024313Z"), revision_date: Some("2024-08-01T20:09:48.024313Z"), projects: Some([SecretWithProjectsInnerProject { id: Some(x), name: Some("x") }]), read: Some(true), write: Some(true) }]), projects: Some([SecretWithProjectsInnerProject { id: Some(x), name: Some("x") }]) }
Reqwest(reqwest::Error { kind: Request, url: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("localhost")), port: Some(4000), path: "/organizations/x/projects", query: None, fragment: None }, source: hyper_util::client::legacy::Error(SendRequest, hyper::Error(User(DispatchGone), "runtime dropped the dispatch task")) })

โฐ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
    team

๐Ÿฆฎ Reviewer guidelines

  • ๐Ÿ‘ (:+1:) or similar for great changes
  • ๐Ÿ“ (:memo:) or โ„น๏ธ (:information_source:) for notes or general info
  • โ“ (:question:) for questions
  • ๐Ÿค” (:thinking:) or ๐Ÿ’ญ (:thought_balloon:) for more open inquiry that's not quite a confirmed
    issue and could potentially benefit from discussion
  • ๐ŸŽจ (:art:) for suggestions / improvements
  • โŒ (:x:) or โš ๏ธ (:warning:) for more significant problems or concerns needing attention
  • ๐ŸŒฑ (:seedling:) or โ™ป๏ธ (:recycle:) for future improvements or indications of technical debt
  • โ› (:pick:) for minor or nitpick changes

@coltonhurst coltonhurst added the hold do not merge, do not approve yet label Aug 13, 2024
@coltonhurst coltonhurst self-assigned this Aug 13, 2024
Copy link
Contributor

Logo
Checkmarx One โ€“ Scan Summary & Details โ€“ 50453c69-d3ef-4c43-99a3-69f126e783f4

Fixed Issues

Severity Issue Source File / Package
MEDIUM Privacy_Violation /languages/go/example/example.go: 103

Copy link

codecov bot commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 0% with 30 lines in your changes missing coverage. Please review.

Project coverage is 58.34%. Comparing base (4be1e09) to head (7f1d71f).
Report is 13 commits behind head on main.

Files Patch % Lines
crates/bitwarden-sm/src/secrets/list.rs 0.00% 18 Missing โš ๏ธ
crates/bitwarden-sm/src/projects/list.rs 0.00% 9 Missing โš ๏ธ
crates/bitwarden-sm/src/client_secrets.rs 0.00% 2 Missing โš ๏ธ
crates/bitwarden-sm/src/client_projects.rs 0.00% 1 Missing โš ๏ธ
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #955      +/-   ##
==========================================
- Coverage   58.45%   58.34%   -0.11%     
==========================================
  Files         194      194              
  Lines       13358    13382      +24     
==========================================
  Hits         7808     7808              
- Misses       5550     5574      +24     

โ˜” View full report in Codecov by Sentry.
๐Ÿ“ข Have feedback on the report? Share it here.

coltonhurst added a commit that referenced this pull request Aug 20, 2024
## ๐ŸŽŸ๏ธ Tracking

https://bitwarden.atlassian.net/browse/SM-1413

## ๐Ÿ“” Objective

### The Issue

Through the C FFI, we were running into concurrency issues with multiple
Tokio runtimes.

A draft PR showing the concurrency issue with instructions can be found
[here](#955).

### The Fix

This PR fixes this issue for `bitwarden-c` and `bitwarden-py`, by
preserving the runtime. Thanks @dani-garcia for the fix and working
together on this one!

### Extra

This also refactors the `AccessTokenLogin` type as a follow up for
testing purposes for Go as initiated by
[this](#953) PR.

## โฐ Reminders before review

- Contributor guidelines followed
- All formatters and local linters executed and passed
- Written new unit and / or integration tests where applicable
- Protected functional changes with optionality (feature flags)
- Used internationalization (i18n) for all UI strings
- CI builds passed
- Communicated to DevOps any deployment requirements
- Updated any necessary documentation (Confluence, contributing docs) or
informed the documentation
  team

## ๐Ÿฆฎ Reviewer guidelines

<!-- Suggested interactions but feel free to use (or not) as you desire!
-->

- ๐Ÿ‘ (`:+1:`) or similar for great changes
- ๐Ÿ“ (`:memo:`) or โ„น๏ธ (`:information_source:`) for notes or general info
- โ“ (`:question:`) for questions
- ๐Ÿค” (`:thinking:`) or ๐Ÿ’ญ (`:thought_balloon:`) for more open inquiry
that's not quite a confirmed
  issue and could potentially benefit from discussion
- ๐ŸŽจ (`:art:`) for suggestions / improvements
- โŒ (`:x:`) or โš ๏ธ (`:warning:`) for more significant problems or
concerns needing attention
- ๐ŸŒฑ (`:seedling:`) or โ™ป๏ธ (`:recycle:`) for future improvements or
indications of technical debt
- โ› (`:pick:`) for minor or nitpick changes
@coltonhurst
Copy link
Member Author

The fix has been merged: #981

@coltonhurst coltonhurst deleted the sm/sm-1413-reproducible branch August 26, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hold do not merge, do not approve yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant