-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Avoid null refs in BlazorIgntior when Disposed #14341
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to throw ObjectDisposedException if used after disposed? Or is this for calls that might come from the server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to throw ObjectDisposedException if used after disposed? Or is this for calls that might come from the server?
Not sure I follow. When would we throw the ObjectDisposedException? |
When checking for Disposed? |
Discussed offline. Pranav is doing the right thing. |
@@ -108,7 +108,7 @@ private async Task<(IWebDriver browser, ILogs log)> CreateBrowserAsync(string co | |||
var instance = await SeleniumStandaloneServer.GetInstanceAsync(output); | |||
|
|||
var attempt = 0; | |||
var maxAttempts = 3; | |||
const int maxAttempts = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we doing this now?
@@ -56,7 +56,7 @@ public BlazorClient() | |||
|
|||
private CancellableOperation<CapturedAttachComponentCall> NextAttachComponentReceived { get; set; } | |||
|
|||
private CancellableOperation<CapturedRenderBatch> NextBatchReceived { get; set; } | |||
internal CancellableOperation<CapturedRenderBatch> NextBatchReceived { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this internal?
} while (attempt < maxAttempts); | ||
|
||
// We will never get here. Keeping the compiler happy. | ||
throw new InvalidOperationException("Couldn't create a Selenium remote driver client. The server is unresponsive"); | ||
throw new InvalidOperationException("Couldn't create a Selenium remote driver client. The server is irresponsive"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw new InvalidOperationException("Couldn't create a Selenium remote driver client. The server is irresponsive"); | |
throw new InvalidOperationException("Couldn't create a Selenium remote driver client. The server is anti-responsive"); |
(Joking, but wouldn't a better message be something like "This line of code should not be reachable", given the comment above?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely orthogonal comment, I've seen selenium timeout in some tests, I'm wondering if we have instances where it becomes irresponsible. And if we should change the code that initializes selenium (in the future) to do this check on a per-test basis and switch the server (start a new one) if it detects the current one is not being responsive.
Fixes #14257