Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No (public) API to close loops individually #40

Closed
XeCycle opened this issue Jul 7, 2016 · 8 comments
Closed

No (public) API to close loops individually #40

XeCycle opened this issue Jul 7, 2016 · 8 comments

Comments

@XeCycle
Copy link
Contributor

XeCycle commented Jul 7, 2016

As the public API goes, I can close all loops with as_event_close_loops, but not individually each of them. The immediate problem is that when I attach a uv loop by as_event_set_external_loop, the uv loop is attached some handles that prevents it to exit automatically; but if I call as_event_close_loops, all are closed, of course, which means as loops attached to other uv loops are affected.

Having a look at what is done by as_event_close_loops, this should be easy to support (and sort of "supported" in as_event_internal.h). I did not submit a PR directly because this involves changes to the API, so I would like you to decide that.

@BrianNichols
Copy link
Member

There are performance implications for allowing event loops to be closed separately. Gaps in the array of event loops would be created which needs to be accounted for when assigning async commands to event loops. This implies at least one extra volatile reference.

Most applications initialize all event loops at startup and close all event loops at shutdown. What is
your use-case for closing an event loop at any time?

@XeCycle
Copy link
Contributor Author

XeCycle commented Jul 8, 2016

OK I see your point in implicitly used event loops, so it may involve significant changes.

My use case is like, our app is in two parts, which originally runs as separate processes. And now we want to merge them in a same process, but running on different threads, so that we have lighter message passing between them. What's different is that, one part of it runs as several instances, starting and stopping on demand, and of course we want a graceful shutdown. Well, mostly an all-isolated environment, except for address space.

So, given that all attached loops are managed as a whole, maybe I am looking for an "aerospike_context" or the like, instead of the static variables now in use.

@BrianNichols
Copy link
Member

We will research dynamic event loop sizing as a possible future enhancement. No concrete decision has been made.

@BrianNichols
Copy link
Member

We have decided not to implement dynamic event loop closing for a number of reasons.

  1. as_event_loop_get() round-robin event loop assignment would require a lock. This would incur a performance penalty for all async users.

  2. Handling simultaneous event loop open/close/assignment is a complex process, especially when the close operation lock must transcend multiple iterations (lock, queue event loop close, process event loop close, unlock).

  3. Each as_node instance maintains a pool of sockets for each event loop. If an event loop is closed, the sockets associated with the event loop would need to be assigned to other event loops or just closed. The problem is event loops have no current knowledge of clusters or nodes. This would require the client to maintain a thread-safe list of all opened clusters. The client could then iterate on all nodes in all clusters and close or reassign the associated sockets. This extra overhead is not currently necessary because all clusters/nodes must be closed before closing event loops.

Note that external event loops can be opened (but not closed) dynamically with the current client. First, define a fixed capacity limit with as_event_set_external_loop_capacity(). Then, add external event loops with as_event_set_external_loop() at any time until the capacity is reached.

@XeCycle
Copy link
Contributor Author

XeCycle commented Jul 15, 2016

What about creating random event loops on the heap without adding to the global as_event_loops? So that external loops are not considered in the round-robin choosing, and is completely external.

@BrianNichols
Copy link
Member

The main obstacle is that there is a socket pool for every node/event loop combination. It was done that way so each socket get/put does not have to be thread-safe. #3 is still a problem in your scenario.

It may be possible to add a configuration variable that instructs nodes to provide a connection pool per node only and then use thread-safe (slower) socket get/put. This would mean #3 is not a problem, because event loops could be disassociated from socket pools. We will do some more research to see if that is feasible.

@BrianNichols
Copy link
Member

Supporting two fundamentally different types of async socket pools complicates the code too much. I'm afraid our original decision stands.

@XeCycle
Copy link
Contributor Author

XeCycle commented Jul 16, 2016

Okay I give up, close at your will. Maybe we resort to a single thread for AS access.

BrianNichols added a commit that referenced this issue Apr 4, 2017
CLIENT-629 : Provide a policy to use services-alternate to avoid int-…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants