Skip to content

Add AsynchronizedFileSink for non-blocking PCAP recording #253

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

RaghavRoy145
Copy link

@RaghavRoy145 RaghavRoy145 commented Mar 20, 2025

Motivation:

The current PCAP recording functionality uses SynchronizedFileSink, which blocks the event loop, this patch introduces AsynchronizedFileSink,which uses Swift's concurrency model to perform non-blocking file writes. This patch attempts to address swift-server/async-http-client#239.

This change also ensures that debugging via PCAP recording remains accessible while preventing the event loop from stalling.

Modifications:

- Implemented AsynchronizedFileSink thread-safe, non-blocking writes.
- Provided unit tests to verify correct behavior.

Result:

- Users can enable non-blocking PCAP recording using AsynchronizedFileSink, improving debugging without risking connection pool performance.
- Test coverage ensures the new implementation works as expected.

Motivation:

The current PCAP recording functionality uses SynchronizedFileSink, which blocks the event loop,
this patch attempts an introduction of AsynchronizedFileSink,which uses Swift's concurrency model
to perform non-blocking file writes. This patch attempts to address swift-server/async-http-client#239.

This change also ensures that debugging via PCAP recording remains accessible while preventing the event loop from stalling.
Modifications:

    - Implemented AsynchronizedFileSink thread-safe, non-blocking writes.
    - Provided unit tests to verify correct behavior.

Result:

    - Users can enable non-blocking PCAP recording using AsynchronizedFileSink, improving debugging without risking connection
    pool performance.
    - Test coverage ensures the new implementation works as expected.
@RaghavRoy145 RaghavRoy145 marked this pull request as draft March 20, 2025 14:48
@@ -808,6 +808,131 @@ extension NIOWritePCAPHandler {
}
}
}

public final class AsynchronizedFileSink {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a better version of this uses _NIOFileSystem to provide the async file I/O APIs. That will also avoid the unsafe code. @glbrntt how do we feel about taking that dependency here?

Copy link
Author

Choose a reason for hiding this comment

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

Oh I didn't consider this, mostly took inspiration from the implementation of SynchronizedFileSink, though I don't think using the filesystem instead would be too tough. Let me know!

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little hesitant to do it before we've made the FilePath changes as that may well have unavoidable API breaks.

@RaghavRoy145 RaghavRoy145 changed the title Add AsynchronizedFileSink for non-blocking PCAP recording [WIP] Add AsynchronizedFileSink for non-blocking PCAP recording Mar 22, 2025
@RaghavRoy145 RaghavRoy145 marked this pull request as ready for review March 22, 2025 17:45
@FranzBusch FranzBusch requested a review from Lukasa May 30, 2025 12:14
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