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

[wasm] rewrite crypto-worker to typescript #73073

Merged
merged 2 commits into from
Aug 6, 2022

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Jul 29, 2022

  • rewrite to TS and add type of InitCryptoMessageData
  • in the crypto-worker we forward the logs to WS based on config.diagnosticTracing
  • returned some console writes in hope they will prevented the deadlock.

Moved some changes from this PR to #73468 and #73073

@ghost
Copy link

ghost commented Jul 29, 2022

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

Issue Details
  • split logging code into separate file logging.ts
  • moved readSymbolMapFile() to startup.ts
  • in the crypto-worker we forward the logs to WS only on Debug build of runtime. We should not do it in production.
  • fixed bug in setup_proxy_console() which didn't really copy the original log and error functions of the console and caused recursion on error.

Fixes #72941

Author: pavelsavara
Assignees: pavelsavara
Labels:

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

Milestone: 7.0.0

src/mono/wasm/runtime/logging.ts Outdated Show resolved Hide resolved
src/mono/wasm/runtime/workers/dotnet-crypto-worker.js Outdated Show resolved Hide resolved
src/mono/wasm/runtime/logging.ts Outdated Show resolved Hide resolved
src/mono/wasm/runtime/logging.ts Outdated Show resolved Hide resolved
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

thanks for converting the crypto worker to TS!

src/mono/wasm/runtime/workers/dotnet-crypto-worker.ts Outdated Show resolved Hide resolved
src/mono/wasm/runtime/workers/dotnet-crypto-worker.ts Outdated Show resolved Hide resolved
@pavelsavara
Copy link
Member Author

I wrote and then spoke to @javiercn this morning about how to locate the url of the dotnet-crypto-worker.js during Blazor startup. How they could influence the location of the file.
We agreed it would be ideal if they could give us the pre-resolved URL as config.asset.

I added new resource type js-module-crypto and js-module-threads, for which Blazor could configure it's asset resolvedUrl.
I also changed the detection of blazor startup sequence to not be disabled by presence of config, but only by config.assets having some assembly in it.

@radical I know you guys also spoke about it.
@lambdageek MT will also need asset for it's dotnet.worker.js I also included it in this PR too.

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

Why is this changing from slice? I think slice reads better.

because slice makes copy, the constructor make a view. Unless I made some mistake.

@eerhardt
Copy link
Member

eerhardt commented Aug 1, 2022

because slice makes copy, the constructor make a view. Unless I made some mistake.

Ah, that's a difference between JS and .NET. Looks like what we want is subarray: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/subarray

@lambdageek
Copy link
Member

lambdageek commented Aug 1, 2022

@lambdageek MT will also need asset for it's dotnet.worker.js I also included it in this PR too.

I'm not familiar with how the assets stuff works... Emscripten just does this:

 allocateUnusedWorker: function() {
  if (!Module["locateFile"]) {
   PThread.unusedWorkers.push(new Worker(new URL("dotnet.worker.js", import.meta.url)));
   return;
  }
  var pthreadMainJs = locateFile("dotnet.worker.js");
  PThread.unusedWorkers.push(new Worker(pthreadMainJs));
 },

but I don't see any changes to locateFile in this PR


Update I suppose we could replace PThread.allocateUnusedWorker with one that goes through the asset mechanism

@pavelsavara
Copy link
Member Author

Update I suppose we could replace PThread.allocateUnusedWorker with one that goes through the asset mechanism

Right, different PR

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@eerhardt
Copy link
Member

eerhardt commented Aug 5, 2022

have you seen the subtlecrypto tests taking more than 15mins on CI?

No - locally on my codespaces VM they take around 70-90 seconds.

@pavelsavara
Copy link
Member Author

have you seen the subtlecrypto tests taking more than 15mins on CI?

No - locally on my codespaces VM they take around 70-90 seconds.

On my windows, when it works the it's 2 minutes.
Total: 2038, Errors: 0, Failed: 0, Skipped: 1, Time: 126.22934s

But I saw it deadlock multiple times, now I'm trying to reproduce it on main, with just the logging fixed here #73468

@eerhardt
Copy link
Member

eerhardt commented Aug 5, 2022

now I'm trying to reproduce it on main, with just the logging fixed here

Yeah, one concern I have with this PR is that you are trying to change too much at once. It is super hard to know what went wrong when making multiple changes in a single PR.

@pavelsavara
Copy link
Member Author

pavelsavara commented Aug 5, 2022

I was able to reproduce the deadlock on this branch locally on my windows
It takes many iterations of

dotnet.exe build /p:TargetOS=Browser /p:TargetArchitecture=wasm /p:Configuration=Debug /t:Test src\libraries\System.Security.Cryptography\tests /p:Scenario=WasmTestOnBrowser /p:UseSubtleCryptoForTests=true /p:WasmTestAppArgs="-class System.Security.Cryptography.Encryption.Aes.Tests.AesContractTests" 

image

@pavelsavara pavelsavara changed the title [wasm] asset loading for subtle crypto + logging [wasm] rewrite crypto-worker to typescript Aug 5, 2022
@pavelsavara
Copy link
Member Author

It is super hard to know what went wrong when making multiple changes in a single PR.

Moved some changes from this PR to #73468 and #73073 and left only rewrite to TS here.

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

LGTM - thanks for splitting this up!

@pavelsavara
Copy link
Member Author

@eerhardt are you guys able to have look at the deadlock ? do you need more info?

@eerhardt
Copy link
Member

eerhardt commented Aug 5, 2022

I haven't been able to look at the deadlock. Does it happen in main, or only with your change?

@pavelsavara
Copy link
Member Author

Probably on main as well, but the fix to logging will help you debugging it without XHarness. Otherwise your chrome will crash with OOM.

@eerhardt
Copy link
Member

eerhardt commented Aug 5, 2022

Probably on main as well

I haven't seen a test failure report in a CI, and the tests have been running for over a month.

…ccess and preventing the deadlock. This is RC1 stop-gap.
@pavelsavara
Copy link
Member Author

pavelsavara commented Aug 5, 2022

Returned some console writes (which I commented out 10 days ago) in hope they prevented the deadlock. I think they were serializing the access to shared buffer. Perhaps we could find better solution later.

@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 7ade2b4 into dotnet:main Aug 6, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 5, 2022
@pavelsavara pavelsavara deleted the fix_crypto_crash 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.

5 participants