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

Bug29085 microcommits rebase2 #1024

Conversation

Labels
None yet
Projects
None yet
4 participants
@mikeperry-tor
Copy link
Contributor

@mikeperry-tor mikeperry-tor commented May 15, 2019

No description provided.

mikeperry-tor added 15 commits May 15, 2019
This commit moves the padding state limit checks and the padding rate limit
checks out of the token removal codepath, and causes all three functions to
get called from a single circpad_machine_count_nonpadding_sent() function.

It does not change functionality.
This commit moves code that updates the state length and padding limit counts
out from the callback to its own function, for clarity.

It does not change functionality.
Deliver nonpadding events instead of calling token removal functions.
Since RTT estimation is disabled in our default machines, this should not be
reached, but it may prove helpful in the future.
…oval explicit.

Make this relationship and all the checks be part of its own function.
Everything is cleaner that way.
Our other tests tested state lengths against padding packets, and token counts
against non-padding packets. This test checks state lengths against
non-padding packets (and also padding packets too), and checks token counts
against padding packets (and also non-padding packets too).

The next three commits are needed to make this test pass (it found 3 bugs).
Yay?
We need to check here because otherwise we can try to schedule padding with no
tokens left upon the receipt of a padding event when our bins just became
empty.
Free circpad machine to pass leaksanitizer in CI.
…e to a github auto-rebase of a PR

Because github PRs choose the most recent origin/master at the time of the PR
(and for any fixups pushed to a PR later to send to CI), there are tons of
conflicts and unexpected practracker issues.

This means CI can suddenly fail after fixups to a branch that pass locally.

Then CI fails and we have to close and re-open the PR.
@coveralls
Copy link

@coveralls coveralls commented May 15, 2019

Pull Request Test Coverage Report for Build 5017

  • 54 of 58 (93.1%) changed or added relevant lines in 1 file are covered.
  • 1258 unchanged lines in 43 files lost coverage.
  • Overall coverage decreased (-1.7%) to 62.46%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/core/or/circuitpadding.c 54 58 93.1%
Files with Coverage Reduction New Missed Lines %
src/feature/hs/hs_service.c 1 73.68%
src/feature/dirclient/dirclient.c 1 18.37%
src/feature/nodelist/nodelist.c 2 71.88%
src/feature/nodelist/microdesc.c 2 72.11%
src/lib/buf/buffers.c 2 91.03%
src/feature/client/addressmap.c 2 60.54%
src/feature/rend/rendcache.c 2 93.02%
src/lib/evloop/token_bucket.h 2 66.67%
src/lib/fs/files.c 2 67.21%
src/feature/dircache/consdiffmgr.c 2 89.81%
Totals Coverage Status
Change from base Build 5006: -1.7%
Covered Lines: 46573
Relevant Lines: 74564

💛 - Coveralls

@mikeperry-tor mikeperry-tor mentioned this pull request May 15, 2019
… in use.

Rate limit rtt failure loglines and make them bugs.
asn-d6
Copy link
Member

asn-d6 commented on 0a10fc6 May 15, 2019

Hmm, this is the second time we get this kind of bug, and it seems like really hard to detect except if a test stumbles upon it. How can we improve it? Perhaps we should de-macro this and turn it into a type-safe function with appropriate naming? Also when should we use one and not the other in general? I'm still not sure.

@torproject-pusher torproject-pusher deleted the branch torproject:master May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment