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: improved locking of assembler state #307

Merged
merged 7 commits into from
Oct 31, 2023
Merged

fix: improved locking of assembler state #307

merged 7 commits into from
Oct 31, 2023

Conversation

loshz
Copy link
Contributor

@loshz loshz commented Oct 29, 2023

Which problem is this PR solving?

Short description of the changes

During the debugging of suspected load testing issues, the following bugs were found and fixed:

  • Switch from sync.Map to regular map w/ mutex 3b8a008
  • Ensure stats accessed concurrently are explicitly atomic fe2ebd3
  • Remove unused sync.Waitgroup from TCP stream factory 2e039ad

How to verify that this has the expected result

On my local Linux machine, I compiled the main branch and the changes in this PR, and ran them concurrently while sending several thousand HTTP requests to a random netcat server. The results are identical:

Main:

DBG TCP assembler stats IPdefrag=0 active_streams=0 conn_rejected_FSM=0 event_queue_length=500 goroutines=6 rejected_FSM=0 rejected_Options=0 source_dropped=0 source_if_dropped=0 source_received=4000 total_TCP_bytes=97500 total_streams=500 uptime_ms=25450
6:33PM DBG Stopping TCP assembler assembler_page_usage="pageCache: used: 0:" closed=500

PR:

DBG TCP assembler stats IPdefrag=0 active_streams=0 conn_rejected_FSM=0 event_queue_length=500 goroutines=6 rejected_FSM=0 rejected_Options=0 source_dropped=0 source_if_dropped=0 source_received=4000 total_TCP_bytes=97500 total_streams=500 uptime_ms=20920
6:33PM DBG Stopping TCP assembler assembler_page_usage="pageCache: used: 0:" closed=500

@loshz loshz requested a review from a team October 29, 2023 01:37
@loshz loshz changed the title Load testing fixes fix: small assembler reliability updates Oct 29, 2023
@JamieDanielson JamieDanielson added type: bug Something isn't working version: bump patch A PR with release-worthy changes and is backwards-compatible. labels Oct 30, 2023
@robbkidd
Copy link
Member

I've got a couple commits to propose tack onto these improvements:

  • reuse the return vars within the function (b7762e2)
  • include recording the packet count when we have a match (5b4cec8)
    • I think we missed adding that to both places prior to your updates in this PR. 🤭

Copy link
Member

@robbkidd robbkidd left a comment

Choose a reason for hiding this comment

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

I gave you bad code, @loshz. 😬

assemblers/http_matcher.go Outdated Show resolved Hide resolved
@robbkidd robbkidd changed the title fix: small assembler reliability updates fix: improved locking of assembler state Oct 31, 2023
@robbkidd robbkidd merged commit 5cc35d2 into honeycombio:main Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working version: bump patch A PR with release-worthy changes and is backwards-compatible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants