andrewjstone / amy Goto Github PK

The type amy::channel::ChannelError is unexposed by the crate, making it difficult to properly handle errors created by the channel types.

Oneshot was a holdover from the original design of Amy where arbitrary types could be registered instead of just integer ids. It was required because of the ownership transfer that needed to occur on the type between the poller thread and the registrar thread. However, thinking about use cases now and how I've been using amy it appears that oneshot is completely unnecessary. Removing it removes pretty much all needs to call reregister on sockets, which in turn removes an extra syscall after each read or write event.

In order to prevent extra hashmap lookups we can encode the type of registered file descriptor (socket, timer, user event) in the unique id of the event. This could also be made part of the public API so a user could quickly determine the type as well and not have to do lookups in multiple tables.

It's unlikely that 2^61 registered events is a real limitation as opposed to 2^64. Extra bits can be saved for future use.

Indicate socket, timer, channel receiver, etc...

The reasoning of "Sockets should only be deregistered when the caller is done with them." makes sense, however this prevents use cases where sockets are transferred between polls. Is this limitation intentional?

The purpose of this is to allow a single threaded event loop to be run without an intermediary process. All server operations can be performed on one thread, including serialization/deserialization, send and receive.

In order for this to work in a larger system where requests are forwarded over channels to other threads and responses received on the event loop thread, we need to be able to poll on channel receivers so the event loop thread can be woken up when it receives responses to send back to clients.

The implementation will be as follows:

Create Sender/Receiver type wrappers around std::sync::mpsc channels. Sending on an amy channel will cause the the event loop to be woken up. There will be a single fd backed by eventfd/kqueue used to wake up the poller. There will also be an atomic bool shared between the registrar and poller.

When a send occurs, the sender will do the normal tx.send() then do a CAS on the atomic bool to see if the there is already a pending wakeup. If there is no pending wakeup the bool will be set to true and the registrar will write to the eventfd/kqueue fd in order to trigger the wakeup. Otherwise, no action will be taken.

When the poller is woken up it will do a CAS on the atomic bool before it does any reads. If there are pending receives it will add the awakener fd to the notification set. The caller to poll will then do a try_read on all channels to get all pending messages.

Note that this strategy can cause spurious wakeups if the sending and receiving threads race and the poll thread has read all messages on the channels before the CAS on the sending thread is successful. However, it maintains its safety and has the following benefit of not requiring a system call to write to the event fd on every send. An occasional spurious wakeup is better than a guaranteed syscall on every send.

There is also an optimization to prevent spurious wakeups. When all reads are completed the poller can recheck the atomic bool and return before calling epoll/kevent.

Right now the api uses a usize for ids. Make it typesafe and the code easier to read.

Some 64 bit assumptions about bitwidth are made in the timerfd code, preventing it from compiling on 32 bit systems.

while the two tests in the repo help a bit, amy is nonetheless an opinionated library and more example usage and documentation would certainly help (at least my) understanding and adoption on how best to incorporate amy into one of my projects.

mind you, this request is also coming from a fairly new rust programmer, and so my inability to completely wrap my head around amy's best practices might be a side-affect of inexperience.

thoughts, @andrewjstone ? 😀

nix-rust/nix#951

The original goal of Amy was to completely decouple the kernel poller from the kernel registrars and
allow them to operate on separate threads with minimal shared state, and no coordination. Registrars
register TCP streams with arbitrary userdata and the poller waits for events on them. The poller
then forwards the events with the streams and userdata back to the owner of one of the registrars
who reads from or writes to the stream using any userdata returned with the notification. Ownership
of the stream and userdata passes from registrar to poller via the kernel and from poller to
registrar via a channel Sender. All state is maintained in the userdata and the connection can be
closed by the thread owning the registrar by simply dropping the stream instead of calling register
again.

The only functionality provided by the above model is register and wait. If a Registrar thread
registers a read event for a socket, but no data is ever sent from the other end, the Registrar will
never hear back. This is fine in some cases, but in the keep-alive case where the connection should
be torn down if no data is sent, there is a problem. The registrar has no way to close the
connection because it never receives back ownership of the socket, and it has no method to instruct
the Poller to remove the connection. Additionally, the user may wish to re-register a socket for a
different event type than is currently registered. This presents a further difficulty, in that the
registrar now not only doesn't have ownership over the socket, but it also doesn't have ownership
over the userdata registered with the kernel poller. If the userdata maintains the state of the
connection, a re-register must re-register with the exact same userdata. This means that the
registrar needs to alias a pointer to the same data registered in the kernel which can be passed
back in a notification. There are solutions to this problem, and I have implemented one, but it
relies heavily on atomic variables and unsafe code, and it threatens the other goal of Amy which is
to be small, simple, and auditable with a minimal reliance on unsafe code.

While deregister functionality is easy to implement by allowing a channel from the registrars to the
poller instructing deletion, re-registration is not straightforward at all, and as far as I can tell
relies on an aliased raw pointer to the underlying userdata registered with the kernel poller.
Ideally, as originally planned, there would never be a use case for re-registration, and the
registrar would not have to alias any arbitrary data stored in the kernel. If there is indeed no
justifiable case for re-registration, the problem can be avoided altogether, and the simple solution
for deregistering of sockets using a channel can be implemented. However, the fear is that there is
a legitimate need for re-register functionality, in which case the deletion mechanism should use the
same mechanism and the registrar should not require a channel to the poller at all.

The following is a scenario where I believe re-registration is required. If so this dictates that we
allow this operation or change the implementation to no-longer allow arbitrary userdata as described in the side note below.

A socket is registered  for read + write events and the registrar thread get's back a
notification that the socket is writable. It writes until there is no more to write and
registers the socket as readable. (Note that if it was registered as readable + writable again
it would immediately get a notification indicating that the socket was writable since there is
still room in the write buffer because we did not get an EAGAIN error on write. This would
cause 100% cpu usage if done continuously.) The socket does not become readable, but 
there is more data to write again suddenly. However, at this point the registrar thread does 
not have ownership of the socket. While it knows the buffer is writable, it cannot write until 
the read notification comes in, which may be arbitrarily long. For low-latency services this is
unacceptable. This requires reregistration.

As a side note, it is possible that we maintain ownership of the TcpStream and connection state in a single registrar, and only register the integer fd with the poller along with a unique integer as user data. This however has a major downside, in that it now ties a connection to a single registrar, rather than allowing ownership of connections to be load balanced among different registrar owning threads. It also removes the capability of registering arbitrary userdata. Admittedly, it is unclear how bad this is in practice, since the socket and it's connection state could be borrowed out or temporarily transferred to a thread pool to do the actual reading and writing. It does mean one more hop across a channel, but has the major upside of not requiring any unsafe code, and potentially eliminates all allocations inside Amy itself.

After some testing with amy on kqueue supported platforms it seems as if events are level triggered (due to EV_CLEAR not being set), not edge triggered like epoll. Is this behavior intentional?