Fix race condition with socket close event from peer #2923
Conversation
slfritchie
added
the
changelog - fixed
src/libponyrt/asio/event.c
Outdated
| if(ev == NULL) | ||
| return false; | ||
|
|
||
| return (ev->flags & ASIO_DISPOSABLE) != 0; |
There was a problem hiding this comment.
This should be return (ev->flags == ASIO_DISPOSABLE); because ASIO_DISPOSABLE has a value of 0 as defined in the enum in asio.h and because when ev->flags is assigned ASIO_DISPOSABLE it is assigned as ev->flags = ASIO_DISPOSABLE unsetting any other flags previously assigned.
There was a problem hiding this comment.
Ouch, yes, I'll fix that.
|
Hrm, the intermittent problems with ARM platforms + runtime not shutting down promptly appears to be happening in other PR branches, e.g. #2917 and timeout failure https://circleci.com/gh/ponylang/ponyc/18748. I spent a bit of time Friday trying to get my Raspberry Pi 3 to try to debug, but installing LLVM 3.9.1 correctly on its Raspian distro was frustrating & ended in failure. |
|
@slfritchie if you rebase on master, we should get a smooth ci run this time :) |
There are a couple places where we can be notified that a TCP socket peer has closed the connection on us: 1. _event_notify(), via an event from the ASIO thread. 2. _pending_reads(), where we try to read from the socket (via the @pony_os_recv()? FFI call which is partial!) and get an error. If we discover the remote peer's close by path #2, then sometime later the ASIO thread may notify us of the close via path #1. Both paths will call `@pony_asio_event_unsubscribe(event)`. There's a sanity check + `pony_assert(0)` inside of the Pony runtime to prevent double calls to `@pony_asio_event_unsubscribe(event)`. However, `pony_assert()` will only act if using a Pony "debug" build of the runtime. So, in non-debug use (i.e., normal day-to-day use), nobody notices the problem: it's only visible when using a debug build (probably because you're debugging some *other* problem). This commit adds a comment + check to avoid double-calls to `@pony_asio_event_unsubscribe(event)`. It's done by adding a new Pony API FFI call, `@pony_asio_event_get_disposable`, that peeks into the event's flag status (and *not* the `flags` argument to `_event_notify()`!) to check if it's safe to unsubscribe. Many thanks to @dipinhora for discussing the merits of how the heck to fix this and to @nisanharamati for being curious about, "Why is this thing sometimes crashing, but only when using a debug build of Pony?"
Fix Windows linking problem
slfritchie
force-pushed
the
socket-close-asio-race
branch
from
be75fbc to
f1c5956
Compare
|
Thanks @mfelsche, I've rebased and am waiting for the CI results to roll in. |
|
Any additional comments prior to merging? |
There are a couple places where we can be notified that a TCP
socket peer has closed the connection on us:
(via the @pony_os_recv()? FFI call which is partial!)
and get an error.
If we discover the remote peer's close by path #2, then sometime
later the ASIO thread may notify us of the close via path #1.
Both paths will call
@pony_asio_event_unsubscribe(event). There'sa sanity check +
pony_assert(0)inside of the Pony runtime toprevent double calls to
@pony_asio_event_unsubscribe(event).However,
pony_assert()will only act if using a Pony "debug" buildof the runtime. So, in non-debug use (i.e., normal day-to-day use),
nobody notices the problem: it's only visible when using a debug
build (probably because you're debugging some other problem).
This commit adds a comment + check to avoid double-calls to
@pony_asio_event_unsubscribe(event). It's done by adding a newPony API FFI call,
@pony_asio_event_get_disposable, that peeksinto the event's flag status (and not the
flagsargument to_event_notify()!) to check if it's safe to unsubscribe.Many thanks to @dipinhora for discussing the merits of how the
heck to fix this and to @nisanharamati for being curious about,
"Why is this thing sometimes crashing, but only when using a
debug build of Pony?"