Problem: poller can segfault when modified from registered handler. #219
Conversation
It is possible that a user would like to add/remove sockets from handlers. As handlers and poll items might be removed while not being processed yet - we have a segfault situation. Provided unit test `remove_from_handler` demonstrates the problem. Solution: Modify internal poll item data structure only after processing of events is finished. Please not that events processing path performance remains the same when there are no modification (add/remove) to the poller (no rebuild) - main real use case. As an effect of changes `size()` method has been removed as it does not represent any meaningful information anymore. There are active and pending (waiting for rebuild) poll items so two different sizes. User can easily track on their side number of registered sockets if original size information is needed. `wait` method returns number of processed sockets now. It might be useful information to a user for no extra cost.
Pull Request Test Coverage Report for Build 69
💛 - Coveralls |
|
|
||
| throw error_t (); | ||
| } | ||
|
|
||
| size_t size () |
There was a problem hiding this comment.
I don't get why removing this method is necessary. There are several other options: get the size before pending changes, get the size after pending changes, provide getters for both.
There was a problem hiding this comment.
what value does it bring to the user? Would it not be more confusing than useful?
poller is not really a collection, you can not iterate through it etc. and user needs to store info about registered socket already in order to successfully unregistered it later.
There was a problem hiding this comment.
Maybe the size is not a relevant information, but the information if the poller is empty or not is, since this affects how wait may be called the next time. Therefore, this information should take into account that pending changes are applied. (I am not sure how you implemented handling of pending changes, but I think that there can only be pending changes if empty is called during an active call of wait, right? Actually, such reentrant calls are not relevant in the use cases I had in mind previously).
Overall I would be fine with removing the size method if an empty method is added instead.
There was a problem hiding this comment.
..but empty has the same problem as size. For example:
zmq::poller_t poller;
poller.add (socket, ZMQ_POLLIN, [&](short events) {
poller.remove (socket);
bool empty = poller.empty();
// hmmm
});
What would you expect empty to be here? You are in the poller's event processing loop but just has removed last socket. If you still insist on adding empty or size I would go for true in the example above as it reflects latest user's actions.
There was a problem hiding this comment.
just created a PR for that.
|
I am now quite sure that it makes sense to split poller_t into two layers: one that only manages socket registrations, and a separate one on top of that that knows about callback handlers. |
| @@ -9,7 +9,6 @@ | |||
| TEST(poller, create_destroy) | |||
| { | |||
| zmq::poller_t poller; | |||
| ASSERT_EQ(0u, poller.size ()); | |||
There was a problem hiding this comment.
Something should be asserted here.
| a.reset (); | ||
| ASSERT_EQ(0u, b.size ()); |
There was a problem hiding this comment.
There must be some assertion here. Or maybe the move operations of poller_t should be deleted entirely.
There was a problem hiding this comment.
It is perfectly valid that unit test does not assert and runs without a crash/exception but if maintainers decide otherwise I am fine with that.
There was a problem hiding this comment.
I completely agree in principle, but I think in this case there should be some assertion.
This test would accept a move constructor that is a no-op...
But maybe it is fine if proper assertion are added for the non-empty case.
But still, I am not sure if it makes sense to provide move operations at all. If one wants to move a poller, they could use a unique_ptr to a poller_t instead.
There was a problem hiding this comment.
Everything inside the poller is movable already so it make it easy to keep poller movable too. Created a new PR that fixes moves operations and makes them compiler generated for us.
| a.reset (); | ||
| ASSERT_EQ(1u, b.size ()); |
There was a problem hiding this comment.
Something must be asserted here. If there are no query operations, wait should be called on both pollers to ensure that only b has an effect.
As disscussed on zeromq#219 PR bringing back `size` method and adding `empty` for completeness.
It is possible that a user would like to add/remove sockets from
handlers. As handlers and poll items might be removed while not
being processed yet - we have a segfault situation.
Provided unit test
remove_from_handlerdemonstrates the problem.Solution: Modify internal poll item data structure only after processing
of events is finished.
Please note that events processing path performance remains the same when there are
no modification (add/remove) to the poller (no rebuild) - main use case.
As an effect of changes
sizemethod has been removed as it does notrepresent any meaningful information anymore. There are active and pending
(waiting for rebuild) poll items so two different sizes. User can
easily track on their side number of registered sockets if original size
information is needed.
waitmethod returns number of processed sockets now. It might beuseful information to a user for no extra cost.