EchoServer tests occasionally crash on Windows #2
Assignees
Labels
os-windows
P1
A high priority bug; for example, a single project is unusable or has many test failures
Comments
|
Added Fixed label. |
madsager
added
Type-Defect
P1
Closed
This was referenced Jun 3, 2015
This was referenced Jun 3, 2015
Closed
copybara-service bot
pushed a commit
that referenced
this issue
Jan 16, 2024
#1: https://dart-review.googlesource.com/c/sdk/+/345363 #1R: https://dart-review.googlesource.com/c/sdk/+/345780 Change-Id: I4fe2b4cb879aee6cdccef0ff233ac86e8d0765ec Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/345827 Reviewed-by: Brian Wilkerson <brianwilkerson@google.com> Reviewed-by: Phil Quitslund <pquitslund@google.com> Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
copybara-service bot
pushed a commit
that referenced
this issue
Feb 23, 2024
Change-Id: Icabef092ce55f981bc6a21819e3733340f037155 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/354043 Commit-Queue: Konstantin Shcheglov <scheglov@google.com> Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
copybara-service bot
pushed a commit
that referenced
this issue
Mar 18, 2024
During my work on dart-lang/language#3648, I ran into some trybot failures with an exception that looked like this: RangeError (index): Invalid value: Valid value range is empty: -1 #0 List.[] (dart:core-patch/growable_array.dart:264:36) #1 List.removeLast (dart:core-patch/growable_array.dart:336:20) #2 InferenceContext.popFunctionBodyContext (package:analyzer/src/generated/resolver.dart:124:33) #3 ResolverVisitor.visitBlockFunctionBody (package:analyzer/src/generated/resolver.dart:1890:38) Some quick reading of the code revealed that `popFunctionBodyContext` always removed a single entry from a list, and `pushFunctionBodyContext` always added a single entry to it. The two calls were always paired up using a straightforward try/finally pattern that seemed like it should guarantee proper nesting, making this exception impossible: try { inferenceContext.pushFunctionBodyContext(...); ... } finally { ... inferenceContext.popFunctionBodyContext(node); } After a lot of headscratching and experimenting, I eventually figured out what was happening: an exception was being thrown during `pushFunctionBodyContext`, _before_ it had a chance to add an entry to the list. But since the exception happened inside the `try` block, the call to `popFunctionBodyContext` would happen anyway. As a result, the pop would fail, causing its own exception, and the exception that was the original source of the problem would be lost. This seemed like a code smell to me: where possible, the clean-up logic in `finally` clauses should be simple enough that it can always succeed, without causing an exception, even if a previous exception has put data structures in an unexpected state. And I had gained enough familiarity with the code over the course of my debugging to see that what we were doing in those `finally` clauses was more complex than necessary: - In the ResolverVisitor, we were pushing and popping a stack of `BodyInferenceContext` objects using the try/finally pattern described above. But we were only ever accessing the top entry on the stack, which meant that the same state could be maintained with a single BodyInferenceContext pointer, and some logic that can't possibly lead to an exception in the `finally` clause: var oldBodyContext = _bodyContext; try { _bodyContext = ...; ... } finally { _bodyContext = oldBodyContext; } - In the ResolverVisitor and the ErrorVerifier, we were also pushing and popping a stack of booleans tracking whether the currently enclosing function (or initializer) has access to `this`. In the ResolverVisitor, this information wasn't being used at all, so it could be safely removed. In the ErrorVerifier, it was being used, but it was possible to simplify it in a similar way, so that it was tracked with a single boolean (`_hasAccessToThis`), rather than a stack. Simplifying this logic brings several advantages: - As noted above, since it's now impossible for an exception to occur in the `finally` clause, exceptions occurring in the `try` clause won't get lost, making debugging easier. - The code should be more performant, since it no longer requires auxiliary heap-allocated stacks. - The code is (IMHO) easier to understand, since the try/catch pattern for maintaining the new `_bodyContext` and `_hasAccessToThis` fields just involves saving a field in a local variable (and restoring it later), rather than calling out to separate classes. Change-Id: I61ae80fb28a69760ea0b2856a6954b4a68cfcbe1 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/358200 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Reviewed-by: Brian Wilkerson <brianwilkerson@google.com> Commit-Queue: Paul Berry <paulberry@google.com>
This issue was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The EchoServer tests occasionally hits what should be an unreachable path.
The text was updated successfully, but these errors were encountered: