Skip to content

Tolerate short writes to output clients - #329

Draft
mutability wants to merge 2 commits into
devfrom
short-writes
Draft

mutability wants to merge 2 commits into
devfrom
short-writes

Conversation

@mutability

Copy link
Copy Markdown

See flightaware/piaware#130 for some background - faup1090 would get stuck in a restart loop if the system pipe buffers were sufficiently small that it got a short write (or EAGAIN) to stdout.

The existing dump1090 code just drops any client which doesn't fully accept a write when dump1090 wants to flush the writer. This PR tries to be a bit smarter and provide some internal buffering where possible, without completely restructuring the one-shared-buffer approach:

There is still a single output buffer per service, shared by all clients of that service. Each client now also has out_index, the index into the output buffer of the first unwritten byte for that particular client.

When writing data, out_index is incremented per-client to reflect bytes successfully written by that client. Periodically (at the end of each flush) we find the first byte in the output buffer that needs to be retained (this is just the minimum out_index value across all clients of the service) and discard the start of the buffer up to that point, adjusting client out_index values appropriately.

When generating data into the output buffer, if we run out of space, we flush data to clients, and disconnect any client which is sufficiently far behind that we don't have at least net_output_flush_size free space. This is what disconnects slow clients that are seeing short writes and not catching up, eventually. It also ensures that we don't do repeated small flushes if there's one almost-full slow client, because we free up net_output_flush_size space per flush, we can generate up to that much more data before a new flush is forced.

We're effectively using the output buffer as an extra local buffer for short writes, so make it somewhat bigger too (32k) to handle bursts.

Also some infrastructure tweaks to make sure the flush configuration semantics stay approximately the same, and some naming / signedness cleanups.

builds, no testing done yet!


PR checklist

  • I certify that this PR meets the requirements of the DCO
  • This PR has a base branch of dev (not master)
  • This PR does not contain generative AI output

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.

1 participant