Skip to content

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

@pavelsavara pavelsavara requested review from eerhardt and radical August 1, 2022 10:13
@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!

@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