Skip to content

Conversation

@pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Aug 7, 2022

  • more locking before manipulation with the channel state.
  • more asserts about expected states
  • removed extra console.debug logging
  • renamed wait method to make it clearer what we are waiting for

Fixes crypto deadlock: Log on main

@pavelsavara pavelsavara added this to the 7.0.0 milestone Aug 7, 2022
@pavelsavara pavelsavara requested review from eerhardt and radical August 7, 2022 19:21
@pavelsavara pavelsavara requested a review from lewing as a code owner August 7, 2022 19:21
@pavelsavara pavelsavara self-assigned this Aug 7, 2022
@ghost
Copy link

ghost commented Aug 7, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details
  • more locking before manipulation with the channel state.
  • more asserts about expected states
  • removed extra console.debug logging
  • renamed wait method to make it clearer what we are waiting for
Author: pavelsavara
Assignees: pavelsavara
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript

Milestone: 7.0.0

@pavelsavara
Copy link
Member Author

CI test with multiple workitems on Helix is #73629

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Overall this looks good to me. Thanks for fixing this!

I just had a few nit-ish comments.

@radical
Copy link
Member

radical commented Aug 10, 2022

re:having-spin-or-nonblocking-in-fn-name, I added those to make it obvious to spot cases where they might get used in incorrect contexts. For example, using a spin wait for a state inside a lock, which could cause a deadlock because the other side is waiting for that lock, so it can change the state.

renamed the subtle-crypto.ts file, because that's not a worker
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara merged commit bb97114 into dotnet:main Aug 11, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 10, 2022
@pavelsavara pavelsavara deleted the wasm_crypto_deadlock_fix branch November 29, 2022 16:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants