Skip to content
This repository has been 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

Conversation

saghul
Copy link
Member

@saghul saghul commented Nov 27, 2014

No description provided.

as follows:

~~~~
int uv_read(uv_read_t* req, uv_stream_t* handle, uv_buf_t[] bufs, unsigned int nbufs, uv_read_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.

Long line.

EDIT: Is there a guideline for that? I would suggest 80 columns.

Copy link
Member Author

Choose a reason for hiding this comment

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

For now, I kept them between 80 and 100, except the code blocks, but I can fix that.

@saghul
Copy link
Member Author

saghul commented Nov 28, 2014

Some stuff that needs clarification:

  • @piscisaureus: can we have a uv_stream_poll thing easily on Windows while still having the other mechanism?
  • @bnoordhuis: I'm not sure we want to reduce everything to uv_stream_poll + reading or writing inline, we'd loose the ability to queue writes, which is nice. Now, we could however get rid of uv_read_alloc, maybe that's what you meant? Also, maybe we could fold uv_read and uv_try_read into one. If you want to do it inline just pass NULL as the request and the callback. Thoughts?

A random note about edge triggering: if we ever plan to use edge triggered i/o, we need a good story for uv_read. When the user uses uv_stream_poll + uv_try_read she can just keep calling it until EAGAIN, but when uv_read is used should we somehow signal the user in case we didn't get EAGAIN when reading, and thus there is more data awaiting? Or maybe it doesn't matter, because we'll try to read inline when uv_read is called anyway...

@piscisaureus
Copy link

@saghul

can we have a uv_stream_poll thing easily on Windows while still having the other mechanism

Depends - it's possible to poll for readable, for some types of streams, most notably TCP connected streams, UDP sockets, and (with some caveats) pipes.

Polling for writable is more problematic. Also note that there may be stream types (file streams) for which readable is not a meaningful event.

I'd like to get rid of reading with an alloc callback. Instead, I would suggest that we support the poll+try_read pattern for the streams that support it. There should be an API to test whether a particular stream can be polled. This enables the embedder to use that strategy when it's the most efficient, and fall back to uv_read() with a pre-allocated buffer when needed.


### A note on uv_cancel

Gradually, `uv_cancel` needs to be improved to allow for cancelling any kind of requests.

Choose a reason for hiding this comment

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

uv_cancel short-circuits an operation, but the callback still gets called (with ECANCELED as the status). That makes it useful for canceling things that might otherwise take a long time, however it doesn't allow the embedder forget about an operation entirely.

That makes it rather pointless to uv_cancel an uv_callback; the uv_callback never takes a long time so uv_cancel would merely change the status code but not do anything else.

It is not always possible to cancel an operation and also prevent it's callback from executing. But sometimes it is possible - think timers, or -indeed- an uv_callback. We should probably have a separate api for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes it rather pointless to uv_cancel an uv_callback; the uv_callback never takes a long time so uv_cancel would merely change the status code but not do anything else.

You're right, I'm not sure what I was thinking here...

It is not always possible to cancel an operation and also prevent it's callback from executing. But sometimes it is possible - think timers, or -indeed- an uv_callback. We should probably have a separate api for this.

I think we could leave the API we have for uv_cancel like it is, for now. For timers, it does make sense to be able to cancel them, and the callback would be called with status == UV_ECANCELED, so you know it didn't actually kick in.

Ideally it would be nice to avoid getting the callback called with UV_ECANCELLED in case uv_cancel returned "true", but I'm not sure this can be done in all cases.

@bnoordhuis
Copy link
Member

I'm not sure we want to reduce everything to uv_stream_poll + reading or writing inline, we'd loose the ability to queue writes, which is nice. Now, we could however get rid of uv_read_alloc, maybe that's what you meant?

That's not quite what I meant but I didn't phrase it very well. My thinking was that we should be able to implement uv_read() in terms of uv_stream_poll() + uv_alloc_cb() + uv_try_read(); ditto for uv_write().

Also, maybe we could fold uv_read and uv_try_read into one. If you want to do it inline just pass NULL as the request and the callback.

I like that.

@saghul
Copy link
Member Author

saghul commented Dec 1, 2014

I'd like to get rid of reading with an alloc callback. Instead, I would suggest that we support the poll+try_read pattern for the streams that support it. There should be an API to test whether a particular stream can be polled. This enables the embedder to use that strategy when it's the most efficient, and fall back to uv_read() with a pre-allocated buffer when needed.

Agreed. I guess that the return code from uv_stream_poll could be used for that: if it returns ENOTSUP, you would fallback.

- Remove note on uv_cancel
- Remove uv_try_read
- Remove uv_read_alloc
- uv_read with NULL callback == former uv_try_read
- Clarified that uv_callback requests cannot be cancelled
- Removed status from uv_callback's callback
- Clarify when timers can fail
@saghul
Copy link
Member Author

saghul commented Dec 1, 2014

Added another round of fixes, please take a look when you can. Thanks!

@bnoordhuis
Copy link
Member

LGTM

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?

@Matthias247
Copy link

Hi,
just a comment from someone who looked through here by acccident and curiosity:
This API looks quite a lot of how boost asio works (without the C++parts) which I used a lot in one of my projects.
I found there the ability to mix asynchronous reads with non-blocking immediate reads quite useful. But I think this is covered by this proposal too (by passing a null callback)?
E.g. I have a TCP protocol which transports messages which consist of a 4 byte header containing the payload length and then the payload. I found at the lowest latency way to deal with that in asio is to do an async read (with callback) for the header and if then know the length to do perform a non-blocking immediate read for the payload. In most situations this will directly return and avoid the latency of a whole iteration through the eventloop. If there's really no content I get EAGAIN and start the async read operation instead.

What you might want for the async read operation is to request it to either callback once any data is received or only when exactly the required amount of data is received.

Afaik polling for read readiness on windows can be done by starting asynchronous 0-byte read operations with IOCP.

uv_stream_t* handle,
const uv_buf_t[] bufs,
unsigned int nbufs,
uv_read_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.

typo?

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 :-)

@saghul
Copy link
Member Author

saghul commented Dec 4, 2014

I found there the ability to mix asynchronous reads with non-blocking immediate reads quite useful. But I think this is covered by this proposal too (by passing a null callback)?

@Matthias247 yes, it will be covered. Maybe not exactly using a NULL callback, put the possibility will be there. We are ironing out API details.

@saghul
Copy link
Member Author

saghul commented Dec 4, 2014

@trevnorris thanks for the feedback, pushed some fixes.

@davidfowl
Copy link

@saghul What's the status of this proposal? Is it dead?

@saghul
Copy link
Member Author

saghul commented Apr 26, 2017

It is still a goal of mine, but this plan was too ambitious and too many things will break. So we are taking smaller steps. Also, we found some bumps on the way and some of the devs (myself, for one) have less time these days to work on it.

@davidfowl
Copy link

@saghul I'm interested in pull based reads. I saw that was planned for 2.0 and this was the only thing I could find with details.

Just FYI, I work for Microsoft on the ASP.NET Core team and we own a web server with a libuv based transport (https://github.com/aspnet/KestrelHttpServer).

@creationix
Copy link

@davidfowl I did at one point experiment with wrapping existing libuv with a C layer that exposed it as pull based. I never could get it very efficient, but depending on your requirements and scope, something like that might work. I wanted pull based since luajit's FFI doesn't do well with C callbacks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants