Skip to content

Commit

Permalink
Fix race condition with socket close event from peer
Browse files Browse the repository at this point in the history
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?"
  • Loading branch information
slfritchie committed Oct 19, 2018
1 parent 4e3c621 commit dd9acda
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 2 deletions.
12 changes: 10 additions & 2 deletions packages/net/tcp_connection.pony
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use @pony_asio_event_unsubscribe[None](event: AsioEventID)
use @pony_asio_event_resubscribe_read[None](event: AsioEventID)
use @pony_asio_event_resubscribe_write[None](event: AsioEventID)
use @pony_asio_event_destroy[None](event: AsioEventID)
use @pony_asio_event_get_disposable[Bool](event: AsioEventID)
use @pony_asio_event_set_writeable[None](event: AsioEventID, writeable: Bool)
use @pony_asio_event_set_readable[None](event: AsioEventID, readable: Bool)

Expand Down Expand Up @@ -495,8 +496,15 @@ actor TCPConnection
_notify_connecting()
end
else
// We're already connected, unsubscribe the event and close.
@pony_asio_event_unsubscribe(event)
// There is a possibility that a non-Windows system has
// already unsubscribed this event already. (Windows might
// be vulnerable to this race, too, I'm not sure.) It's a
// bug to do a second time. Look at the disposable status
// of the event (not the flags that this behavior's args!)
// to see if it's ok to unsubscribe.
if not @pony_asio_event_get_disposable(event) then
@pony_asio_event_unsubscribe(event)
end
@pony_os_socket_close[None](fd)
_try_shutdown()
end
Expand Down
8 changes: 8 additions & 0 deletions src/libponyrt/asio/event.c
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,14 @@ PONY_API int pony_asio_event_fd(asio_event_t* ev)
return ev->fd;
}

PONY_API bool pony_asio_event_get_disposable(asio_event_t* ev)
{
if(ev == NULL)
return false;

return (ev->flags & ASIO_DISPOSABLE) != 0;
}

PONY_API bool pony_asio_event_get_writeable(asio_event_t* ev)
{
if(ev == NULL)
Expand Down

0 comments on commit dd9acda

Please sign in to comment.