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.

Sign up for GitHub

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

Jump to bottom

Move cs_vSend into SocketSendData and resolve RecordBytesSent lock inconsistency #19673

Closed
wants to merge 3 commits into from
Closed

Move cs_vSend into SocketSendData and resolve RecordBytesSent lock inconsistency #19673

wants to merge 3 commits into from

Commits on Nov 3, 2020

  1. cleanup: Clean PushMessage 

    This commit brings PushMessage in line with the style guide, with a few
simple renames, some changes to if statements, and the removal of an
unnecessary "== true".

Easter Egg - serializedHeader has the weird "in between" naming
convention: no szHungarianNotation but still camelCase.
    @troygiorshev
    troygiorshev committed Nov 3, 2020
    Configuration menu
    Copy the full SHA
    75e6280 View commit details
    Browse the repository at this point in the history

  2. Add missing lock annotations 

    These variables are already guarded by cs_vSend, they are just missing
the annotations.
    @troygiorshev
    troygiorshev committed Nov 3, 2020
    Configuration menu
    Copy the full SHA
    92897d4 View commit details
    Browse the repository at this point in the history

  3. Move cs_vSend lock into SocketSendData 

    SocketSendData requires cs_vSend to be locked, but it places this
responsibility with the caller.  This is unnecessary, unclear, and has
caused us to accidentally add RecordBytesSent under the lock.

This commit fixes both problems.

Justification is needed in the case of PushMessage.  It is ok that we
let go of cs_vSend for a moment before locking it again and calling
SocketSendData.  In the case where optimistic_send == false, this lock
is let go of for a long time between adding messages to vSendMsg and the
call to SocketSendData (in SocketHandler).  As corrected in the comment
change, optimistic_send represents whether vSendMsg *was* empty, not
*is* empty (as, of course, we've just added to it).

If SocketHandler grabs cs_vSend in the middle then SocketSendData will
be empty when we call it in PushMessage.  As suggested by the "if
(bytes_sent)" check, this is fine.
    @troygiorshev
    troygiorshev committed Nov 3, 2020
    Configuration menu
    Copy the full SHA
    9d47d6d View commit details
    Browse the repository at this point in the history