Skip to content

proxy: disable buffering for gateway recording#23

Open
recursivetree wants to merge 1 commit intoRoachbones:mainfrom
recursivetree:improve-gateway-recording
Open

proxy: disable buffering for gateway recording#23
recursivetree wants to merge 1 commit intoRoachbones:mainfrom
recursivetree:improve-gateway-recording

Conversation

@recursivetree
Copy link
Collaborator

@recursivetree recursivetree commented Apr 27, 2025

I've noticed the same issue as in #16, but with gateway traffic. This PR fixes this by flushing after every message.

Since the websocket data is binary and not text, we cannot use pythons line buffering mode. Instead, we flush manually.

Since not flushing has caused some issues with missing entries in my gateway_index file, I've also added logic to detect the highest unused flow id even if some data got lost from the gateway_index.

@Roachbones
Copy link
Owner

Have you noticed any performance impact from this? I remember avoiding constant flushing because I was concerned about ruining websocket performance on computers with hard drives.

@recursivetree
Copy link
Collaborator Author

I haven't noticed an performance impact, but I haven't tested it on harddrives either since I only have SSDs.

Things to consider:

  • the OS also does some caching (but can we rely on it?)
  • alternatively, it might be possible to flush every x seconds or something like that.

@recursivetree
Copy link
Collaborator Author

I've decided to split the non-controversial parts of this PR into #26 so we can discuss this in detail.

If anyone has an HDD and is able to test, I'd appreciate it if you could do so.

@limdingwen
Copy link
Contributor

I have a HDD, but which parts of it need testing?

@recursivetree
Copy link
Collaborator Author

I have a HDD, but which parts of it need testing?

I'm not an expert for which statistics to look at for measuring storage performance. But once we know that, I'd suggest running a test with and without this 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

Comments