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

abstract_tcp_server2: move some things out of a lock #5531

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@moneromooo-monero
Copy link
Contributor

commented May 10, 2019

The lock is meant for the network throttle object only,
and this should help coverity get unconfused

abstract_tcp_server2: move some things out of a lock
The lock is meant for the network throttle object only,
and this should help coverity get unconfused
@vtnerd

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

These functions can be called by multiple threads and are not "in a strand". The variable in the connection context involved in the read/write are "plain" doubles. This isn't thread-safe, you could even get "tearing" of values as a result of this change.

@vtnerd

vtnerd approved these changes May 10, 2019

Copy link
Contributor

left a comment

Ah you know, scratch that, I think this will work.

do_send_chunk is only called from do_send which holds a lock when calling that function. And I think handle_read is only invoked in a strand. Since the values being manipulated differ, this should work. Although the gains have to be like a couple 100 cycles but only when the connection context is not in the cache.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

It is not, but being in that lock will not make them thread safe either. It'd need another lock that callers use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.