[S] Change in dart/sdk[main]: [vm/isolate_api] Fix thread-starting race in threading_test.
Slava Egorov (Gerrit)
Slava Egorov added 2 comments![]( "Open in Gerrit")
I think this fixes a symptom (test failing), not the actual bug (VM deadlocking trying to safepoint?).
bool isHelperInThreadMainWaitingLatchRunning = false;This should not be possible according to the newest spec. All shared variables are supposed to be `final`. Sounds like we are missing some checks :)
Related details
- Alexander Aprelev
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev added 2 comments![]( "Open in Gerrit")
I think this fixes a symptom (test failing), not the actual bug (VM deadlocking trying to safepoint?).
When spawned thread catches unexpected exception it stops responding, resulting in a timeout. The change in test ensures there are no unexpected exceptions thrown.
bool isHelperInThreadMainWaitingLatchRunning = false;This should not be possible according to the newest spec. All shared variables are supposed to be `final`. Sounds like we are missing some checks :)
Somehow I missed that. There are multiple such variables in this and other tests that do communicate between isolates using `vm:shared` fields.
Related details
- Slava Egorov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Slava Egorov added 1 comment![]( "Open in Gerrit")
Alexander AprelevI think this fixes a symptom (test failing), not the actual bug (VM deadlocking trying to safepoint?).
When spawned thread catches unexpected exception it stops responding, resulting in a timeout. The change in test ensures there are no unexpected exceptions thrown.
I am speaking about this output from the test:
```
Attempt:11 waiting for isolate helper to check in
Attempt:12 waiting for isolate helper to check in
Attempt:13 waiting for isolate helper to check in
Attempt:14 waiting for isolate helper to check in
Attempt:15 waiting for isolate helper to check in
```
This is timeout in safepointing - which indicates some sort of deadlock in the machinery we have added. This should not happen - so I think the test right now is revealing some actual bug.
Related details
- Alexander Aprelev
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev added 1 comment![]( "Open in Gerrit")
Alexander AprelevI think this fixes a symptom (test failing), not the actual bug (VM deadlocking trying to safepoint?).
Slava EgorovWhen spawned thread catches unexpected exception it stops responding, resulting in a timeout. The change in test ensures there are no unexpected exceptions thrown.
I am speaking about this output from the test:
```
Attempt:11 waiting for isolate helper to check in
Attempt:12 waiting for isolate helper to check in
Attempt:13 waiting for isolate helper to check in
Attempt:14 waiting for isolate helper to check in
Attempt:15 waiting for isolate helper to check in
```This is timeout in safepointing - which indicates some sort of deadlock in the machinery we have added. This should not happen - so I think the test right now is revealing some actual bug.
I believe this is test's `waitLatch` in `threadMainWaitingLatch` that blocks isolate execution. Doesn't look like a bug in the vm.
Related details
- Slava Egorov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Slava Egorov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Slava Egorov
| Code-Review | +1 |
1 comment
Alexander AprelevI think this fixes a symptom (test failing), not the actual bug (VM deadlocking trying to safepoint?).
Slava EgorovWhen spawned thread catches unexpected exception it stops responding, resulting in a timeout. The change in test ensures there are no unexpected exceptions thrown.
Alexander AprelevI am speaking about this output from the test:
```
Attempt:11 waiting for isolate helper to check in
Attempt:12 waiting for isolate helper to check in
Attempt:13 waiting for isolate helper to check in
Attempt:14 waiting for isolate helper to check in
Attempt:15 waiting for isolate helper to check in
```This is timeout in safepointing - which indicates some sort of deadlock in the machinery we have added. This should not happen - so I think the test right now is revealing some actual bug.
I believe this is test's `waitLatch` in `threadMainWaitingLatch` that blocks isolate execution. Doesn't look like a bug in the vm.
Ah, it is not safepointing but VM shutdown, which is blocked by isolate being stuck (Maybe we should update the message to make it clear).
I think it is worth considering if we should introduce a way to gracefully shutdown in such situations (e.g. whether we should make it possible to interrupt such wait).
Related details
- Alexander Aprelev
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev voted and added 3 comments![]( "Open in Gerrit")
Votes added by Alexander Aprelev
| Commit-Queue | +2 |
3 comments
thank you Slava!
Alexander AprelevI think this fixes a symptom (test failing), not the actual bug (VM deadlocking trying to safepoint?).
Slava EgorovWhen spawned thread catches unexpected exception it stops responding, resulting in a timeout. The change in test ensures there are no unexpected exceptions thrown.
Alexander AprelevI am speaking about this output from the test:
```
Attempt:11 waiting for isolate helper to check in
Attempt:12 waiting for isolate helper to check in
Attempt:13 waiting for isolate helper to check in
Attempt:14 waiting for isolate helper to check in
Attempt:15 waiting for isolate helper to check in
```This is timeout in safepointing - which indicates some sort of deadlock in the machinery we have added. This should not happen - so I think the test right now is revealing some actual bug.
Slava EgorovI believe this is test's `waitLatch` in `threadMainWaitingLatch` that blocks isolate execution. Doesn't look like a bug in the vm.
Ah, it is not safepointing but VM shutdown, which is blocked by isolate being stuck (Maybe we should update the message to make it clear).
I think it is worth considering if we should introduce a way to gracefully shutdown in such situations (e.g. whether we should make it possible to interrupt such wait).
bool isHelperInThreadMainWaitingLatchRunning = false;Alexander AprelevThis should not be possible according to the newest spec. All shared variables are supposed to be `final`. Sounds like we are missing some checks :)
Somehow I missed that. There are multiple such variables in this and other tests that do communicate between isolates using `vm:shared` fields.
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
dart-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
dart-...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Change information
[vm/isolate_api] Fix thread-starting race in threading_test.
Fixes https://github.com/dart-lang/sdk/issues/63526
TEST=ci
- M tests/ffi/threading_test.dart
Code-Review: +1 by Slava Egorov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |