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

Fix LogViewer PreRenderQueue error #131

Merged
merged 2 commits into from
Oct 9, 2023
Merged

Fix LogViewer PreRenderQueue error #131

merged 2 commits into from
Oct 9, 2023

Conversation

tlmii
Copy link
Member

@tlmii tlmii commented Oct 9, 2023

Resolves #124

Switch to using a ConcurrentQueue to handle the PreRenderQueue duties.

Also removes an unused method from LogViewer and simplifies the common callstack from WatchLogsAsync -> AddLogEntriesAsync -> WriteLogsAsync to just WatchLogsAsync -> WriteLogsAsync.

@davidfowl
Copy link
Member

Where does the concurrency happen?

@tlmii
Copy link
Member Author

tlmii commented Oct 9, 2023

Where does the concurrency happen?

The async enumerator iteration in WatchLogsAsync can start up before OnAfterRenderAsync is called and finished (there's a small time between when the LogViewer is available to it's parent but before the LogViewer itself is rendered).

If WatchLogsAsync happens to call WriteLogsAsync while OnAfterRenderAsync is iterating the PreRenderQueue, it will try to add another entry to the queue while in the process of iterating the queue.

Since the old code was just a List, and iteration was a foreach, that threw the exception you saw.

Switching to a Queue (or even just switching to use a for loop) would avoid the exception, but since the problem was of the write-while-reading type, it seemed safer to just go ahead and use a ConcurrentQueue. Its use is limited to the very small window of time before the first rendering of the component (and the OnAfterRenderAsync handler) is complete.

@@ -78,19 +73,14 @@
{
_jsModule ??= await JS.InvokeAsync<IJSObjectReference>("import", "/_content/Aspire.Dashboard/Components/Controls/LogViewer.razor.js");

if (_preRenderQueue.Count > 0)
while (_preRenderQueue.Count > 0)
Copy link
Member

Choose a reason for hiding this comment

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

Why not replace the Count > 0 check with the TryDequeue(out logs) instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

My understanding (which could be wrong!) was that "it's really empty" is only one of the reasons that TryDequeue can return false. So that's why I kept the normal check to try again if it wasn't actually empty when the TryDequeue failed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Read through the code for TryDequeue and it looks like you're right. I think some of the descriptions I read about the use of it just weren't precise enough.

@tlmii tlmii merged commit 956b3d7 into dotnet:main Oct 9, 2023
4 checks passed
@tlmii tlmii deleted the dev/fix-logviewer-prerender-write-while-read branch October 9, 2023 21:47
@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2024
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.

Hit an exception navigating around eshop in LogViewer
5 participants