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

BUG:dynamicbanscore deadlock #1509

Merged
merged 1 commit into from
Mar 9, 2020
Merged

BUG:dynamicbanscore deadlock #1509

merged 1 commit into from
Mar 9, 2020

Conversation

lochjin
Copy link
Contributor

@lochjin lochjin commented Jan 2, 2020 •
edited
Loading

@lochjin lochjin mentioned this pull request Jan 2, 2020
Closed
@torkelrogstad
Copy link
Contributor

Nice find! Could you add a test for the String() method? So we make sure the deadlock isn't reintroduced later by someone finding the line you just changed and thinking it makes sense to replace it with the function call.

@lochjin
Copy link
Contributor Author

lochjin commented Feb 8, 2020

Nice find! Could you add a test for the String() method? So we make sure the deadlock isn't reintroduced later by someone finding the line you just changed and thinking it makes sense to replace it with the function call.

OK, no problem. 530ea00

Rjected
Rjected suggested changes Feb 9, 2020
View reviewed changes
connmgr/dynamicbanscore_test.go Outdated
if r != 150 {
t.Errorf("Unexpected result %d after ban score increase.", r)
}
fmt.Println(bs.String())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than using fmt here, you could do t.Log or t.Logf and I think it should still work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍OK. c982d9b

@jakesylvestre
Copy link
Collaborator

jakesylvestre commented Mar 4, 2020 •
edited
Loading

@jcvernaleo (as per #1530)

  • high priority
  • bug

Mergable

jcvernaleo
jcvernaleo approved these changes Mar 6, 2020
View reviewed changes
Copy link
Member

@jcvernaleo jcvernaleo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@jcvernaleo
Copy link
Member

@lochjin would you be able to squash this to a single commit and then I'll merge it?

@lochjin
Copy link
Contributor Author

lochjin commented Mar 6, 2020 •
edited
Loading

@lochjin would you be able to squash this to a single commit and then I'll merge it?

OK

@jakesylvestre
Copy link
Collaborator

Looks good!

jcvernaleo
jcvernaleo approved these changes Mar 9, 2020
View reviewed changes
Copy link
Member

@jcvernaleo jcvernaleo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@jcvernaleo jcvernaleo merged commit 96f3808 into btcsuite:master Mar 9, 2020
@Rjected Rjected mentioned this pull request May 12, 2020
Closed
@zquestz zquestz mentioned this pull request Jul 19, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rjected Rjected Rjected approved these changes

@jakesylvestre jakesylvestre jakesylvestre approved these changes

@cfromknecht cfromknecht cfromknecht approved these changes

@jcvernaleo jcvernaleo jcvernaleo approved these changes

Assignees
No one assigned
Labels
bug high priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lochjin @torkelrogstad @jakesylvestre @jcvernaleo @cfromknecht @Rjected