[S] Change in dart/sdk[main]: [io/socket] Make Socket::fd_ atomic since it can get updated from mul...
Alexander Aprelev (Gerrit)
Alexander Aprelev voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Tess Strickland
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tess Strickland (Gerrit)
Tess Strickland voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tess Strickland
| Code-Review | +1 |
1 comment
Related details
- Alexander Aprelev
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alexander Aprelev
| Commit-Queue | +2 |
1 comment
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Commit Queue submitted the change![]( "Open in Gerrit")
Change information
[io/socket] Make Socket::fd_ atomic since it can get updated from multiple threads.
Follow-up to https://dart.googlesource.com/sdk/+/ba7aaa929f579aaa55a2fc09a098f81df6b27804, sink the "is fd closed" check to avoid multiple reads.
Fixes https://github.com/dart-lang/sdk/issues/61582
TEST=ci
- M runtime/bin/socket.cc
- M runtime/bin/socket.h
- M runtime/bin/socket_base_win.cc
- M runtime/bin/socket_win.cc
Code-Review: +1 by Tess Strickland
| 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")
I am sorry, maybe I am missing something. How does this fix the race itself?
TSAN reported that Builtin_Socket_GetType races with closure of the socket happening on the event handler thread. That's great finding. What does this CL do to *fix* the underlying race? We are still racing - we might get stale FD just as event handler is closing/deleting that thing. Is it not the case?
Related details
Code-Owners
Code-Review
Commit-Message-Has-TEST
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")
I am sorry, maybe I am missing something. How does this fix the race itself?
TSAN reported that Builtin_Socket_GetType races with closure of the socket happening on the event handler thread. That's great finding. What does this CL do to *fix* the underlying race? We are still racing - we might get stale FD just as event handler is closing/deleting that thing. Is it not the case?
The goal of this cl was to fix the crash that happens in case of this race - ensure we don't try to dereference closed fd. It seems that the race itself is unavoidable.
Related details
Code-Owners
Code-Review
Commit-Message-Has-TEST
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 am sorry, maybe I am missing something. How does this fix the race itself?
TSAN reported that Builtin_Socket_GetType races with closure of the socket happening on the event handler thread. That's great finding. What does this CL do to *fix* the underlying race? We are still racing - we might get stale FD just as event handler is closing/deleting that thing. Is it not the case?
The goal of this cl was to fix the crash that happens in case of this race - ensure we don't try to dereference closed fd. It seems that the race itself is unavoidable.
So how does this CL ensure that? The situation is equivalent to
```
Thread 1:
foo = bar.x
use(foo->baz)
Thread 2:
delete bar.x
bar.x = deleted_marker // -1
```
The code became:
```
Thread 1:
foo = bar.x
if (foo == deleted_marker) return;
use(foo->baz)
Thread 2:
delete bar.x
bar.x = nullptr
```
This `if` by no means avoids either the race or a crash because the following sequence if possible:
```
T1: foo = bar.x // != marker
T2: delete bar.x
T1: if (foo == deleted_marker) return; // false
T1: foo->baz // crash
```
It seems that the race itself is unavoidable
I am not sure. `NativeSocket` has `isClosing` which is set when we dispatch `closeCommand` to the eventhandler. So I would imagine that checking that in `_nativeSocketType` before calling `_getSocketType` should help.
Related details
Code-Owners
Code-Review
Commit-Message-Has-TEST
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")
So I would imagine that checking that in _nativeSocketType before calling _getSocketType should help.
Thank you, I think you are right. Somehow I thought that there is no way to guarantee ordering between close message handling done by eventhandler thread and isolate mutator thread accessing socket. https://dart-review.googlesource.com/c/sdk/+/451822
Related details
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |