Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Deferred connection evaluation #41
Deferred connection evaluation #41
Changes from 4 commits
9e3b2d2
4981326
bdc4e01
a86884e
31d3fbc
d16b483
ae55479
a5512d3
fcdccb5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I think the approach of simply copying (or moving out of) the connections is quite cool, as it allows to quickly release the mutex again.
However, this approach conflicts with deleting connections.
When the following happens asynchronously
The connection should either be evaluated before the
disconnect
call returns, or it should not be evaluated, if thedisconnect
call returns beforeevaluateDeferredConnections
is called. As it's highly likely that the connection includes dangling pointers after the call todisconnect
the connection.So if an evaluation is in progress the call to
disconnect
cannot return until the evaluation has completely finished.If you still want to keep the approach of simply moving the connections over to release the mutex early, you could add a second mutex for "disconnect", which would be held longer, whilst the mutex for adding new connections could already be released.
However, this double-locking may be less performant in the end, depending on how many connections are queued, as we now need to lock two mutexes when evaluating and when disconnecting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really have a doubt about this, how i am thinking about is, so follow the order
-> emit a deferred signal
-> disconnect the connection
-> evaluateDeferredConnections
disconnecting the signal after the emit or before the evaluation wouldn't be have any issue, as evaluation depends on the list of connections in the evaluator which has already queued up through the call to emit. Call to disconnect removes the connection from the
m_connections
of the Signal class, which is fine.I can only think about the issue when evaluation or disconnect either be used in mulithreading context. Or i might be thinking wrong?
I am adding the test case for this, to look up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like we discussed with @seanharmer and @lemirep , both approaches are feasible.
Removing queued invocations on disconnect is really a lot better to protect from dangling references.
But as Sean noted, it might just be something that needs to be expected with a new paradigm.
So if you're not sure what to do yet, feel free to explore your initial implementation a bit further (i.e. still evaluating queued invocations even after disconnect).
I'd just like to see the implications of this in use in an actual application.
Basically: How easy is it to foot-gun yourself with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I believe it is dangerous to have the previously emitted connection still be evaluated is:
Imagine that
val
was aint *
allocated withint* val = new int;
I would expect this to then work:
connection.disconnect(); delete val;
Then calling
evaluateDeferredConnections
would still reference val, as it's part of the connected closure.This access would then be a dangling pointer and most likely SegFault, or write to random memory.
That's why I'm concerned about this behavior, you simply cannot guarantee that if you emitted a signal that references some resources those resources aren't referenced anymore after disconnecting.
Though that's exactly what disconnect is meant for in a non-deferred scenario.
The current behavior needs additional synchronization to ensure that both
disconnect
andevaluateDeferredConnections
are called before any resources are freed.Especially as those two calls are likely to happen in completely different threads the synchronization might be very difficult.
On the other hand I also get that it may be unintuitive that a signal emission doesn't actually execute a slot if it's not evaluated before it's disconnected.
Maybe @seanharmer or @lemirep have a preference on this, as they're using KDBindings quite extensively afaik.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation! That does make sense. Now assuming there is no trick to extend the lifetime of closure object, seems like we have to safely synchronize it, or basically make sure that evaluation happen before the call to disconnect.
I understand your point here, but I am concerned about how we used to understand signal, slot emit and disconnect. My intution while looking at code that contains structure like
connectedDeferred -> emit -> disconnect -> evaluateDeferred
, I would like to see it as the classic implementation, means if signal is emitted before disconnection, it should be evaluated , no matter if the call to evaluation is after disconnecting the signal. But sadly that does have the problem with dangling reference. I would also love to see , views from @seanharmer and @lemirep