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 Incorrect timeout error when using FindStringMatch() #86

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Brinenas
Copy link

fix Timeout Error issue: when the fastclock stop running, then next matches will get a wrong fast.current and daeline, thus trigger an incorrect timeout error.

…atches will get a wrong fast.current and daeline, thus trigger an incorrect timeout error.
Copy link

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

LGTM 👍

but I would expect some tests to be provided with the fix. It would avoid errors in any future refactoring

…atches will get a wrong fast.current and daeline, thus trigger an incorrect timeout error.
@Brinenas
Copy link
Author

Add a new UT, If you comment the add code, the ut will failed.

fastclock_test.go Outdated Show resolved Hide resolved
Co-authored-by: ccoVeille <[email protected]>
@dlclark
Copy link
Owner

dlclark commented Aug 21, 2024

Thanks for submitting this. Can you add an integration-level test showing the behavior fix from the API consumer perspective?

@Brinenas
Copy link
Author

Add a new UT for my own case. Maybe you guys can add a more elegant test.

@dlclark
Copy link
Owner

dlclark commented Aug 23, 2024

@Brinenas a couple issues I see:

  • fast.running and fast.start are protected by fast.mu for read/write, so we can't access them without getting the lock
  • I don't think we want to get that lock every time we need to make a deadline because this is an edge case. I've refactored the code to only do this if we already know the deadline needs to be extended.

I did a couple other small tweaks to the test as well. Let me know if you see any issues with my change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants