Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

lep: request all the things #2

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
188 changes: 188 additions & 0 deletions XXX-request-all-the-things.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,188 @@
| Title | Request all the things |
|--------|-------------------------|
| Author | @saghul |
| Status | DRAFT |
| Date | 2014-11-27 07:43:59 |


## Overview

This proposal describes a new approach for dealing with operations in libuv. As of
right now, handles define an entity which is capable of performing certain operations.
These operations are sometimes a result of a request being sent and some other times a
result of a callback (which was passed by the user) being called. This proposal aims
to make this behavior more consistent, by turning several operations that currently
just take a callback into a request form.


### uv_read

(This was previously discussed, but it’s added here for completeness).

Instead of using a callback passed to `uv_read_start`, the plan is to use a `uv_read`
function which performs a single read operation. The initial prototype was defined
as follows:

~~~~
int uv_read(uv_read_t* req,
uv_stream_t* handle,
const uv_buf_t[] bufs,
unsigned int nbufs,
uv_read_cb cb)
~~~~

The read callback is defined as:

~~~~
typedef void (*uv_read_cb)(uv_read_t* req, int status)
~~~~

Implementation details: we probably will want to have some `bufsml` of size 4 where we
copy the structures when the request is created, like `uv_write` does. Thus, the user can
pass a `uv_buf_t` array which is allocated on the stack, as long as the memory in each `buf->base`
is valid until the request callback is called.

Inline reading: if the passed callback `cb` is NULL and there are no more queued read requests

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I disagree. We should not define the "meaning" of omitting a callback arbirtrarily. Instead I'd prefer to use a generic pattern.

The uv_fs_xx class of functions also allows you to omit the callback. There it means that the operation should be blocking.

uv_close() allows one to omit the callback. There it means that the operation is still asynchronous but no callback gets called.

My preference is to say that passing no callback to an async function means "blocking".

In addition, what's weird is that in the case of uv_write() the semantic of the function also changes subtly. Whereas the async version of uv_write() would always write the entire buffer, the "try" variant would only write as much bytes as it could.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, we would be inconsistent there. How about passing a NULL request instead? Alternatively we could keep the uv_try_* variants instead of foldling the functionality into a single API function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on the above, Bert?

an attempt to read inline will be made.


### uv_write and uv_try_write

`uv_write` will be modified to behave just like `uv_read`, that is, try to do the operation
inline if `cb` is NULL, and thus `uv_try_write` will be removed.


### uv_stream_poll

In case `uv_read` and `uv_write` are not enough, another way to read or write on streams
is to get a callback when the stream is readable / writable, and use `uv_read` and `uv_write`
to perform the reads and writes inline (passing a NULL callback). The proposed API for this:

~~~~
int uv_stream_poll(uv_stream_poll_t* req,
uv_stream_t* handle,
int events,
uv_stream_poll cb)
~~~~

`events` would be a mask composed of `UV_READABLE` and / or `UV_WRITABLE`.

The callback is defined as:

~~~~
typedef void (*uv_stream_poll_cb)(uv_stream_poll_t* req, int status)
~~~~


### uv_timeout

Currently libuv implements repeating timers in the form of a handle. The current implementation
does not account for the time taken during the callback, and this has caused some trouble
every now and then, since people have different expectations when it comes to repeating timers.

This proposal removes the timer handle and makes timers a request, which gets its callback
called when the timeout is hit:

~~~~
int uv_timeout(uv_timeout_t* req,
uv_loop_t* loop,
double timeout,
uv_timeout_cb cb)
~~~~

The `timeout` is now expressed as a double. The fractional part will get rounded up
to platform granularity. For example: 1.2345 becomes 1235 ms or 1,234,500 us,
depending on whether the platform supports sub-millisecond precision.

Timers are one shot, so no assumptions are made and repeating timers can be easily
implemented on top (by users).

The callback takes the following form:

~~~~
typedef void (*uv_timeout_cb)(uv_timeout_t* req, int status)
~~~~

The status argument would indicate success or failure. The only possible failure is cancellation,
which would make status == `UV_ECANCELED`.

Implementation detail: Timers will be the first thing to be processed after polling for i/o.


### uv_callback

In certain environments users would like to get a callback called by the event loop, but
scheduling this callback would happen from a different thread. This can be implemented using
`uv_async_t` handles in combination with some sort of thread safe queue, but it’s not
straightforward. Also, many have fallen in the trap of `uv_async_send` coalescing calls,
that is, calling the function X times does not yield the callback being called X times; it’s
called at least once.

`uv_callback` requests will queue the given callback, so that it’s called “as soon as
possible” by the event loop. 2 API calls are provided, in order to make the thread-safe
version explicit:

~~~~
int uv_callback(uv_callback_t* req, uv_loop_t* loop, uv_callback_cb cb)
int uv_callback_threadsafe(uv_callback_t* req, uv_loop_t* loop, uv_callback_cb cb)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not entirely clear to me what the thread-safe version would do. Block until the event loop processes the callback?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was an implementation detail, but maybe it's worth mentioning. The loop keeps track of active requests using a queue, so we cannot really insert anything there from another thread. My idea was to have a separate queue for callback requests which are sent from a different thread, then, as part of the loop processing, those requests are fully initialized and added to the normal queue. This other queue would need to be thread safe, using locking, atomic ops or whatever.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added a note about this.

~~~~

The callback definition:

~~~~
typedef void (*uv_callback_cb)(uv_callback_t* req)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the uv_callback_t would contain a void* data field that will allow me to attach whatever I want.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, just like all requests do. Furthermore, you can embed the structure into one of your own and add as many fields as you want.

~~~~

Implementation detail: since the callback request cannot be safely initialized outside
of the loop thread, when `uv_callback_threadsafe` is used, the request will be put
in a queue which will be processed by the loop at some point, fully initializing the
requests.

The introduction of `uv_callback` would deprecate and remove `uv_async_t` handles.
Now, in some cases it might be desired to just wake up the event loop, and having to
create a request might be too much, thus, the following API call is also proposed:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm experimenting with a new API to allow queue's of work to be done, where the callback of each queue returns an int. If the return value is != 0 then the queue stops being processed. With that, I'd probably have to add a queue complete callback.

Anyway, I got part of the idea from GCD and I'll be implementing it in libnub for testing. This is just an FYI.


`uv_callback` requests cannot be cancelled.

~~~~
void uv_loop_wakeup(uv_loop_t* loop)
~~~~

Which would just wake up the event loop in case it was blocked waiting for i/o.

Implementation detail: the underlying mechanism for waking up the loop will be decided
later on. The current `uv_async_t` machanism could remain (on Unix) or atomic ops
could be used instead.

Note: As a result of this addition, `uv_idle_t` handles will be deprecated an removed.
It may not seem obvious at first, but `uv_callback` achieves the same: the loop won’t block
for i/o if any `uv_callback` request is pending. This becomes even more obvious with the
"pull based’ event loop" proposal.


### uv_accept / uv_listen

Currently there is no way to stop listening for incoming connections. Making the concept
of accepting connections also request based makes the API more consistent and easier
to use: if the user decides so (maybe because the system ran out of file descriptors
and EMFILE erros are returned, foe example) it's possible to stop accepting new
connections by just stopping to create new accept requests.

New API:

~~~~
int uv_listen(uv_stream_t* stream, int backlog)
~~~~

The uv_listen function loses its callback, becoming the equivalent of `listen(2)`.

~~~~
int uv_accept(uv_accept_t* req, uv_stream_t* stream, uv_accept_cb cb)
typedef void (*uv_accept_cb)(uv_accept_t* req, int status)
~~~~

Once a connection is accepted the request callback will be called with status == 0.
The `req->fd` field will contain a `uv_os_fd_t` value, which the user can use together

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That in particular won't work on windows. It is not common to map an open socket to a "CRT fd" and it'd limit the number of open sockets to 2000. An uv_os_handle_t could work though.

Pragmatics and all that, but it has always my belief that if properly designed libuv would not need to expose raw file descriptors / handles to the user, ever. That makes for a better cross-platform abstraction, and it also clarifies the issue of ownership; if OS fds/sockets are completely hidden from the user it's clear that libuv is responsible for managing the resource.

At the same time I am well aware that being religious about this has led to some less than pretty APIs, like the way the uv_listen callback receives incoming connections in version 1.0 makes the user do casts and switches based on the handle type. The infamous "julia pipe" issue can also be partly attributed to this: casually creating and discarding a libuv handle is super unpractical.

So I'm +1 if changed to uv_os_handle_t (which is to be defined as HANDLE on windows, and int on unix), but expect me to always be a bit sad about this api.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uv_os_fd_t is already a HANDLE: https://github.com/libuv/libuv/blob/master/include/uv-win.h#L233 ✨ I added it for uv_fileno which doesn't return CRT fds, but Windows HANDLE thingies. So, we seem to be on the same page here 👍

with `uv_tcp_open` for example. (This needs further investigation to verify it would
work in all cases).