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

fix: short-term fix for issue #322 #333

Closed
wants to merge 1 commit into from
Closed

fix: short-term fix for issue #322 #333

wants to merge 1 commit into from

Conversation

sigua-cs
Copy link
Contributor

Description

Max value for Host.Counter. Short-term fix for issue #322

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Checklist

  • Linter passes correctly
  • Add tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary

Does this introduce a breaking change?

  • Yes
  • No

Further comments

@render
Copy link

render bot commented Apr 29, 2023

@sigua-cs
Copy link
Contributor Author

Does it make sense to remove s.host.dec()

s.host.dec()

since the decrement will be called by defer s.dec()
defer s.dec()

for the host that returns BadGateway?

const tmpDir = "/tmp"
const (
tmpDir = "/tmp" // tmpDir temporary path to store ongoing queries results
defaultHostCounter = uint32((1 << 32) - 1) // default value for Host.Counter; Short-term fix issue #322
Copy link
Collaborator

Choose a reason for hiding this comment

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

be-careful, with such a big value you'll likely overflow.

You should put 1 << 31

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll test 1<<31
However, when it's 1<<32 s.host.inc() will be called

if err := s.incQueued(); err != nil {

as result value will be 0

Copy link
Collaborator

@mga-chka mga-chka Apr 30, 2023

Choose a reason for hiding this comment

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

yes, and if 2 queries happens at the same time the value will be 2^32 + 1 (i.e overflow)

@sigua-cs
Copy link
Contributor Author

sigua-cs commented May 2, 2023

After internal discussion closing this PR. I'll reopen another one with short-term fix

@sigua-cs sigua-cs closed this May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants