[Fuchsia] EventHandler: epoll -> ports v2

[Fuchsia] EventHandler: epoll -> ports v2

This CL rewrites the EventHandler on Fuchsia to use
the low-level ports system call API instead of the
epoll() emulation in musl. This will allow the
Magenta team to remove the epoll() emulation and
the deprecated system call API it is based on.

The new ports API doesn't provide epoll()-like
edge-triggering that clients of the EventHandler
expect. Therefore, this CL emulates it by adding
a level of indirection through a new "IOHandle"
object, which the socket code now uses instead
of file descriptors. This is similar to what we do
on Windows. See the comment at the top of
eventhandler_fuchsia.cc for more details.

Fuchsia issue US-251

R=asiva@google.com

Committed: https://github.com/dart-lang/sdk/commit/bad6b64062912b3dedf80cd0c9bd9c0c0ec3bbcb

[zra, 2017-06-19 16:09:58]

Description was changed from

==========
[Fuchsia] EventHandler: epoll -> ports v2
==========

to

==========
[Fuchsia] EventHandler: epoll -> ports v2

This CL rewrites the EventHandler on Fuchsia to use
the low-level ports system call API instead of the
epoll() emulation in musl. This will allow the
Magenta team to remove the epoll() emulation and
the deprecated system call API it is based on.

The new ports API doesn't provide epoll()-like
edge-triggering that clients of the EventHandler
expect. Therefore, this CL emulates it by adding
a level of indirection through a new "IOHandle"
object, which the socket code now uses instead
of file descriptors. This is similar to what we do
on Windows. See the comment at the top of
eventhandler_fuchsia.cc for more details.
==========

[zra, 2017-06-19 18:03:56]

Description was changed from

==========
[Fuchsia] EventHandler: epoll -> ports v2

This CL rewrites the EventHandler on Fuchsia to use
the low-level ports system call API instead of the
epoll() emulation in musl. This will allow the
Magenta team to remove the epoll() emulation and
the deprecated system call API it is based on.

The new ports API doesn't provide epoll()-like
edge-triggering that clients of the EventHandler
expect. Therefore, this CL emulates it by adding
a level of indirection through a new "IOHandle"
object, which the socket code now uses instead
of file descriptors. This is similar to what we do
on Windows. See the comment at the top of
eventhandler_fuchsia.cc for more details.
==========

to

==========
[Fuchsia] EventHandler: epoll -> ports v2

This CL rewrites the EventHandler on Fuchsia to use
the low-level ports system call API instead of the
epoll() emulation in musl. This will allow the
Magenta team to remove the epoll() emulation and
the deprecated system call API it is based on.

The new ports API doesn't provide epoll()-like
edge-triggering that clients of the EventHandler
expect. Therefore, this CL emulates it by adding
a level of indirection through a new "IOHandle"
object, which the socket code now uses instead
of file descriptors. This is similar to what we do
on Windows. See the comment at the top of
eventhandler_fuchsia.cc for more details.

Fuchsia issue US-251
==========

[zra, 2017-06-19 18:06:51]

zra@google.com changed reviewers:
+ asiva@google.com, swetland@google.com

[zra, 2017-06-19 18:06:51]

zra@google.com changed reviewers:
+ asiva@google.com, swetland@google.com

[abarth, 2017-06-19 19:02:44]

abarth@google.com changed reviewers:
+ abarth@google.com

[abarth, 2017-06-19 19:02:46]

https://codereview.chromium.org/2910853002/diff/180001/runtime/bin/eventhandl...
File runtime/bin/eventhandler_fuchsia.h (right):

https://codereview.chromium.org/2910853002/diff/180001/runtime/bin/eventhandl...
runtime/bin/eventhandler_fuchsia.h:142: static const uint64_t
kInterruptPacketKey = 0xda47da47;
You might as well just use the value 0x1 for this purpose.  This value makes it
look like you're trying not to collide with other things, but the value appears
to be just used in a user packet, which you're fully in control of.

[zra, 2017-06-19 21:04:28]

Changed to use user packets instead of asserting user signals on handles.

https://codereview.chromium.org/2910853002/diff/180001/runtime/bin/eventhandl...
File runtime/bin/eventhandler_fuchsia.h (right):

https://codereview.chromium.org/2910853002/diff/180001/runtime/bin/eventhandl...
runtime/bin/eventhandler_fuchsia.h:142: static const uint64_t
kInterruptPacketKey = 0xda47da47;
On 2017/06/19 19:02:45, abarth wrote:
> You might as well just use the value 0x1 for this purpose.  This value makes
it
> look like you're trying not to collide with other things, but the value
appears
> to be just used in a user packet, which you're fully in control of.

Done.

[zra, 2017-06-20 21:57:16]

Updated to call async wait directly from the Dart thread after reads and writes.

PTAL.

[abarth, 2017-06-20 22:07:34]

https://codereview.chromium.org/2910853002/diff/220001/runtime/bin/eventhandl...
File runtime/bin/eventhandler_fuchsia.h (right):

https://codereview.chromium.org/2910853002/diff/220001/runtime/bin/eventhandl...
runtime/bin/eventhandler_fuchsia.h:71: mx_status_t
ReadySignalsLocked(mx_signals_t* ready_signals);
I didn't find any callers of this function.

[abarth, 2017-06-20 22:07:54]

I'm not an expert on this code, but it looks like a good direction.

[zra, 2017-06-20 22:22:27]

https://codereview.chromium.org/2910853002/diff/220001/runtime/bin/eventhandl...
File runtime/bin/eventhandler_fuchsia.h (right):

https://codereview.chromium.org/2910853002/diff/220001/runtime/bin/eventhandl...
runtime/bin/eventhandler_fuchsia.h:71: mx_status_t
ReadySignalsLocked(mx_signals_t* ready_signals);
On 2017/06/20 22:07:34, abarth wrote:
> I didn't find any callers of this function.

Left over from previous approach. Removed.

[swetland, 2017-06-21 00:08:55]

Just a note on cancel behaviour

[swetland, 2017-06-21 00:09:51]

https://codereview.chromium.org/2910853002/diff/240001/runtime/bin/eventhandl...
File runtime/bin/eventhandler_fuchsia.cc (right):

https://codereview.chromium.org/2910853002/diff/240001/runtime/bin/eventhandl...
runtime/bin/eventhandler_fuchsia.cc:216: if ((status != MX_OK) && (status !=
MX_ERR_NOT_FOUND)) {
I haven't charted out everything here, but just an FYI, if you are multithreaded
and call port_cancel(..., key):

OK indicates that the wait either had not triggered, or had triggered but the
packet had not been read out of the port (and was removed).  

NOT_FOUND indicates that the wait was not pending (so either it triggered
already or key was never waited upon) and there was no pending packet to remove
(so if the wait did trigger its packet has already been read).

[siva, 2017-06-21 00:48:10]

https://codereview.chromium.org/2910853002/diff/240001/runtime/bin/eventhandl...
File runtime/bin/eventhandler_fuchsia.cc (right):

https://codereview.chromium.org/2910853002/diff/240001/runtime/bin/eventhandl...
runtime/bin/eventhandler_fuchsia.cc:84: LOG_INFO("IOHandle::Read: fd = %ld. read
%ld bytes\n", fd_, read_bytes);
Should we check for error from read?

For errors should we just return the error without resubscribing to more read
events?

https://codereview.chromium.org/2910853002/diff/240001/runtime/bin/eventhandl...
runtime/bin/eventhandler_fuchsia.cc:99: LOG_INFO("IOHandle::Write: fd = %ld.
wrote %ld bytes\n", fd_, written_bytes);
Ditto comment here about errors from write.

https://codereview.chromium.org/2910853002/diff/240001/runtime/bin/eventhandl...
runtime/bin/eventhandler_fuchsia.cc:113: LOG_INFO("IOHandle::Accept: fd = %ld.
socket = %ld\n", fd_, socket);
ditto comment about errors returned from accept.

https://codereview.chromium.org/2910853002/diff/240001/runtime/bin/eventhandl...
runtime/bin/eventhandler_fuchsia.cc:166: uint64_t key) {
ASSERT that the thread holds the mutex lock.

https://codereview.chromium.org/2910853002/diff/240001/runtime/bin/socket_bas...
File runtime/bin/socket_base_fuchsia.cc (right):

https://codereview.chromium.org/2910853002/diff/240001/runtime/bin/socket_bas...
runtime/bin/socket_base_fuchsia.cc:91: IOHandle* handle =
reinterpret_cast<IOHandle*>(fd);
ASSERT(handle->fd() >= 0);

[zra, 2017-06-21 06:43:39]

https://codereview.chromium.org/2910853002/diff/240001/runtime/bin/eventhandl...
File runtime/bin/eventhandler_fuchsia.cc (right):

https://codereview.chromium.org/2910853002/diff/240001/runtime/bin/eventhandl...
runtime/bin/eventhandler_fuchsia.cc:84: LOG_INFO("IOHandle::Read: fd = %ld. read
%ld bytes\n", fd_, read_bytes);
On 2017/06/21 00:48:10, siva wrote:
> Should we check for error from read?
> 
> For errors should we just return the error without resubscribing to more read
> events?

read(), write(), and accept() can "fail" for asynchronous sockets with
EWOULDBLOCK, in which case we need to resubscribe. I'd rather leave the logic
for deciding which errors are real errors, and which can be ignored in the layer
above. I'll add a comment.

https://codereview.chromium.org/2910853002/diff/240001/runtime/bin/eventhandl...
runtime/bin/eventhandler_fuchsia.cc:99: LOG_INFO("IOHandle::Write: fd = %ld.
wrote %ld bytes\n", fd_, written_bytes);
On 2017/06/21 00:48:10, siva wrote:
> Ditto comment here about errors from write.

Acknowledged.

https://codereview.chromium.org/2910853002/diff/240001/runtime/bin/eventhandl...
runtime/bin/eventhandler_fuchsia.cc:113: LOG_INFO("IOHandle::Accept: fd = %ld.
socket = %ld\n", fd_, socket);
On 2017/06/21 00:48:10, siva wrote:
> ditto comment about errors returned from accept.

Acknowledged.

https://codereview.chromium.org/2910853002/diff/240001/runtime/bin/eventhandl...
runtime/bin/eventhandler_fuchsia.cc:166: uint64_t key) {
On 2017/06/21 00:48:10, siva wrote:
> ASSERT that the thread holds the mutex lock.

The Mutex under bin/ doesn't track the owner. It looks like it would be useful
in a few other places though, so I should take a look at adding that in a
different CL.

https://codereview.chromium.org/2910853002/diff/240001/runtime/bin/eventhandl...
runtime/bin/eventhandler_fuchsia.cc:216: if ((status != MX_OK) && (status !=
MX_ERR_NOT_FOUND)) {
On 2017/06/21 00:09:51, swetland wrote:
> I haven't charted out everything here, but just an FYI, if you are
multithreaded
> and call port_cancel(..., key):
> 
> OK indicates that the wait either had not triggered, or had triggered but the
> packet had not been read out of the port (and was removed).  

Just to make sure I understand: If there is a pending packet and I cancel, when
I wait again for the signal that caused the packet to be queued, the packet will
be queued again (assuming the condition still exists)?

> NOT_FOUND indicates that the wait was not pending (so either it triggered
> already or key was never waited upon) and there was no pending packet to
remove
> (so if the wait did trigger its packet has already been read).
> 

Acknowledged.

https://codereview.chromium.org/2910853002/diff/240001/runtime/bin/socket_bas...
File runtime/bin/socket_base_fuchsia.cc (right):

https://codereview.chromium.org/2910853002/diff/240001/runtime/bin/socket_bas...
runtime/bin/socket_base_fuchsia.cc:91: IOHandle* handle =
reinterpret_cast<IOHandle*>(fd);
On 2017/06/21 00:48:10, siva wrote:
> ASSERT(handle->fd() >= 0);

Done.

[swetland, 2017-06-21 20:07:29]

https://codereview.chromium.org/2910853002/diff/240001/runtime/bin/eventhandl...
File runtime/bin/eventhandler_fuchsia.cc (right):

https://codereview.chromium.org/2910853002/diff/240001/runtime/bin/eventhandl...
runtime/bin/eventhandler_fuchsia.cc:216: if ((status != MX_OK) && (status !=
MX_ERR_NOT_FOUND)) {
Yes. Whenever you object_wait_async(), if any of the signals you are waiting on
are asserted at the moment you make the syscall, a packet will immediately be
queued.

(It's technically not the same packet as the one that was removed when you
canceled the previous wait, but the contents are the same if the underlying
conditions have not changed).

[zra, 2017-06-23 14:53:22]

@asiva did you have any more comments?

[siva, 2017-06-23 15:08:27]

lgtm

https://codereview.chromium.org/2910853002/diff/280001/runtime/bin/eventhandl...
File runtime/bin/eventhandler_fuchsia.cc (right):

https://codereview.chromium.org/2910853002/diff/280001/runtime/bin/eventhandl...
runtime/bin/eventhandler_fuchsia.cc:108: // Resubscribe to to write events.
typo 'to to'

[zra, 2017-06-23 17:31:38]

https://codereview.chromium.org/2910853002/diff/280001/runtime/bin/eventhandl...
File runtime/bin/eventhandler_fuchsia.cc (right):

https://codereview.chromium.org/2910853002/diff/280001/runtime/bin/eventhandl...
runtime/bin/eventhandler_fuchsia.cc:108: // Resubscribe to to write events.
On 2017/06/23 15:08:27, siva wrote:
> typo 'to to'

Done.

[zra, 2017-06-23 17:41:25]

Description was changed from

==========
[Fuchsia] EventHandler: epoll -> ports v2

This CL rewrites the EventHandler on Fuchsia to use
the low-level ports system call API instead of the
epoll() emulation in musl. This will allow the
Magenta team to remove the epoll() emulation and
the deprecated system call API it is based on.

The new ports API doesn't provide epoll()-like
edge-triggering that clients of the EventHandler
expect. Therefore, this CL emulates it by adding
a level of indirection through a new "IOHandle"
object, which the socket code now uses instead
of file descriptors. This is similar to what we do
on Windows. See the comment at the top of
eventhandler_fuchsia.cc for more details.

Fuchsia issue US-251
==========

to

==========
[Fuchsia] EventHandler: epoll -> ports v2

This CL rewrites the EventHandler on Fuchsia to use
the low-level ports system call API instead of the
epoll() emulation in musl. This will allow the
Magenta team to remove the epoll() emulation and
the deprecated system call API it is based on.

The new ports API doesn't provide epoll()-like
edge-triggering that clients of the EventHandler
expect. Therefore, this CL emulates it by adding
a level of indirection through a new "IOHandle"
object, which the socket code now uses instead
of file descriptors. This is similar to what we do
on Windows. See the comment at the top of
eventhandler_fuchsia.cc for more details.

Fuchsia issue US-251

R=asiva@google.com

Committed:
https://github.com/dart-lang/sdk/commit/bad6b64062912b3dedf80cd0c9bd9c0c0ec3bbcb
==========

[zra, 2017-06-23 17:41:27]

Committed patchset #16 (id:300001) manually as
bad6b64062912b3dedf80cd0c9bd9c0c0ec3bbcb (presubmit successful).