[PM-20615] Only process incoming messages once#264
Conversation
|
Sorry for assigning this to you too @dani-garcia, but I don't think anyone else has experience with Send + Sync |
|
Great job, no security vulnerabilities found in this Pull Request |
|
I'm debugging the tests now but apparently they only fail when you run Edit: Fixed |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #264 +/- ##
==========================================
- Coverage 70.32% 70.29% -0.03%
==========================================
Files 217 216 -1
Lines 16992 17060 +68
==========================================
+ Hits 11949 11992 +43
- Misses 5043 5068 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
(cherry picked from commit 875c3a9)
(cherry picked from commit 875c3a9)
dani-garcia
left a comment
dani-garcia
left a comment
There was a problem hiding this comment.
Some comments about the locking we're doing, but otherwise LGTM
| @@ -1,3 +1,8 @@ | |||
| #[cfg(all(target_arch = "wasm32", not(feature = "wasm")))] | |||
| compile_error!( | |||
| "The `wasm` feature must be enabled to use the `bitwarden-ipc` crate in a WebAssembly environment." | |||
There was a problem hiding this comment.
bitwarden-threading doesn't have any feature defined in cargo.toml, does it? Why is this needed?
I wonder if we should decide whether we want to use target_arch = "wasm32" or feature = "wasm" and just update the whole repo to be consistent.
There was a problem hiding this comment.
I added a wasm feature. I needed to add it because the crates that depend on threading use features and so I had to either convert them to target_arch too or convert threading to use feature.
Yeah, I think features has some advantage but I can't remember what 🙃 So far adding the wasm flag to a non-wasm build (which we do when we run cargo test --all-features) hasn't caused any issues because wasm-bindgen has a goal to always compile on all target and instead throw runtime errors if you try to use it on an unsupported target. But now, we have this thing:
#[cfg(not(target_arch = "wasm32"))]
use tokio::task::spawn_local;
#[cfg(all(target_arch = "wasm32", feature = "wasm"))]
use wasm_bindgen_futures::spawn_local;which is actively used in unit-tests and so all of a sudden we cannot just do
#[cfg(not(featgure = "wasm"))]
use tokio::task::spawn_local;
#[cfg(feature = "wasm")]
use wasm_bindgen_futures::spawn_local;because that would cause wasm_bindgen_futures::spawn_local to be called during a regular unit test which would cause a runtime error.
So either we remove wasm from cargo test (cargo test --all-features --except wasm ???) or we use target_arch
|
2 participants
🎟️ Tracking
📔 Objective
BitwardenClient⏰ Reminders before review
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 confirmedissue 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