paddybyers / node

evented I/O for v8 javascript
http://nodejs.org/
Other
91 stars 24 forks source link

eio does not support multiple loops #14

Open paddybyers opened 12 years ago

paddybyers commented 12 years ago

uv-eio.c contains this:

/* TODO remove me! _/ static uv_loopt main_loop;

This is because the two poll callbacks uv_eio_want_poll and uv_eio_done_poll are called asynchronously without any context, and it is not possible to determine which loop the events are associated with. Normally, this isn't a problem, but it becomes a problem if an application wants to have multiple active loops in a single process.

Support for multiple instances is sought for the Android port, and I'm now running into this problem.

My thought was to put the required information into the eio_req via a new poll_data field, and for this field to be populated as required whenever the calling application constructs the eio_req.

The proposed changes are here.

A perhaps cleaner but more intrusive change would be to add the poll_data as an explicit argument into each of the eio_xxx constructors.

This has been tested with multiple simultaneous node instances in a single process and seems to be working OK.

paddybyers commented 12 years ago

In fact, this isn't working.

Firstly, with the patch as written, the poll_data field isn't being populated until after the req is submitted. This is obviously fixable, but isn't the key problem.

The key problem is that the want_poll callback is called for the first outstanding queued item, and the event loop and thread associated with that first request is then used to process all other pending requests (irrespective of whether or not they belong to that same event loop).

Watch this space.

paddybyers commented 12 years ago

So this is a better attempt:

https://github.com/paddybyers/libuv/commit/3ad1de77598e09ed0da19a61cddac7fff137ee02

The key issue I have is that each eio_req is associated with a specific event loop, and the completion processing for that request can only be performed in the thread associated with that event loop. This applies when that thread is associated with a specific v8 isolate or is attached to a specific java vm, for example.

So I need my want_poll callback to be called on a per-event-loop basis - ie on the occurrence of the first pending response for that loop. The current behaviour just gives me a callback on the first occurrence globally; I can schedule activity when that happens, but I get no further callbacks until that activity is complete. From the outside (ie as a client of eio) I have no way of telling that there are also queued responses associated with my other (idle) event loops.

So my thought is to have a res_queue for each event loop. There is still only one req_queue, one eio thread pool, and only one condition variable to wait on. There can still be a default res_queue, but a req can be associated with a specific res_queue when it is constructed, and the want_poll and done_poll callbacks are then made with reference to that queue. In the patch linked above, there is a struct eio_channel which encapsulates the res_queue and associated user data.

(Unlike my last attempt) this can now run two node instances in separate threads, each running a file io benchmark and the responses are being directed to the correct threads for processing.

I'm waiting to see if there is feedback from libev.