Open
Conversation
Removed mQueue field from sbfTcpConnection. This was being toggled in different threads without synchronisation so was undefined behaviour. I don't believe it provided any functionality since before enqueuing we call bufferevent_disable and after dequeuing we call bufferevent_enable. Removed mDestroyed for the same reason. Reworked how sbfTcpConnection_destroy works. Previously, it would call bufferevent_free on mEvent. However, other threads (in particular, the work queue thread on to which read events are dispatched) could be using the mEvent, causing use after frees (manifesting as hangs, as freed mutexes inside libevent would be locked). Now when calling sbfTcpConnection_destroy, all callbacks are cleared (preventing further events being fired by libevent, hence preventing further messages being enqueued on to the work queue), and then a final item is enqueued on the work queue, which frees the resources of the sbfTcpConnection. To ensure the bufferevent is always alive when the dequeuing is done, when enqueuing it we now increment the bufferevent's reference count, using bufferevent_incref, and after dequeuing it we decrement it using bufferevent_decref (with no incrementing done for the final destroy event enqueued obviously). I believe (though not certain) that clearing the callbacks is thread safe, so no further events will fire after they're cleared (even on other threads). If this is true then we can get rid of the ref counting altogether (including the mRefCount) and just free things only in the final destroy callback which is now enqueued when calling sbfTcpConnection_destroy. One other change is to no longer free the bufferevent whenever an error occurs and we enqueue an error work item on the queue. I think freeing the bufferevent here complicates things and doesn't provide anything useful, it leaves the sbfTcpConnection in a weird state of being half freed and half not freed, and there's no way to create a new bufferevent for an existing sbfTcpConnection. One consequence of the destroy rework is that the sbfQueue used by the sbfTcpConnection must now be freed after the sbfTcpConnection has been freed. This makes more intuitive sense than the opposite (since the sbfTcpConnection uses the queue, it should be freed first), but its now definitely required (since we enqueue when destroying which, if the sbfQueue were already destroyed, would fail the assert when enqueuing) as opposed to before where the queue could be destroyed first and almost never would throw an assertion error (though it would if libevent tried to enqueue something, if there was e.g. activity on a socket, so a sporadic assertion failure would happen before if the queue was destroyed first).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Removed mQueued field from sbfTcpConnection. This was being toggled in
different threads without synchronisation so was undefined behaviour. I
don't believe it provided any functionality since before enqueuing we
call bufferevent_disable and after dequeuing we call bufferevent_enable.
Removed mDestroyed for the same reason.
Reworked how sbfTcpConnection_destroy works. Previously, it would call
bufferevent_free on mEvent. However, other threads (in particular, the
work queue thread on to which read events are dispatched) could be using
the mEvent, causing use after frees (manifesting as hangs, as freed
mutexes inside libevent would be locked).
Now when calling sbfTcpConnection_destroy, all callbacks are cleared
(preventing further events being fired by libevent, hence preventing
further messages being enqueued on to the work queue), and then a final
item is enqueued on the work queue, which frees the resources of the
sbfTcpConnection.
To ensure the bufferevent is always alive when the dequeuing is done,
when enqueuing it we now increment the bufferevent's reference count,
using bufferevent_incref, and after dequeuing it we decrement it using
bufferevent_decref (with no incrementing done for the final destroy
event enqueued obviously).
I believe (though not certain) that clearing the callbacks is thread
safe, so no further events will fire after they're cleared (even on
other threads). If this is true then we can get rid of the ref counting
altogether (including the mRefCount) and just free things only in the
final destroy callback which is now enqueued when calling
sbfTcpConnection_destroy.
One other change is to no longer free the bufferevent whenever an error
occurs and we enqueue an error work item on the queue. I think freeing
the bufferevent here complicates things and doesn't provide anything
useful, it leaves the sbfTcpConnection in a weird state of being
half freed and half not freed, and there's no way to create a new
bufferevent for an existing sbfTcpConnection.
One consequence of the destroy rework is that the sbfQueue used by the
sbfTcpConnection must now be freed after the sbfTcpConnection has been
freed. This makes more intuitive sense than the opposite (since the
sbfTcpConnection uses the queue, it should be freed first), but its now
definitely required (since we enqueue when destroying which, if the
sbfQueue were already destroyed, would fail the assert when enqueuing)
as opposed to before where the queue could be destroyed first and almost
never would throw an assertion error (though it would if libevent tried
to enqueue something, if there was e.g. activity on a socket, so a
sporadic assertion failure would happen before if the queue was
destroyed first).