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

Add async variant of ByteBuffer.withUnsafeReadableBytes. #2694

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ser-0xff
Copy link
Contributor

Add async variant of ByteBuffer.withUnsafeReadableBytes<>.

Motivation:

Would be nice to have a possibility to use byte buffers with swift concurrency.

Modifications:

Add async variant of ByteBuffer.withUnsafeReadableBytes<>.

Result:

Having an async variant of ByteBuffer.withUnsafeReadableBytes<> will give a possibility to call async functions from the argument closure.

@Lukasa
Copy link
Contributor

Lukasa commented Mar 27, 2024

Hi @ser-0xff, what's the intended use-case of this function? It generally gives me the heeby-jeebies because it can easily lead to sendability or law of exclusivity violations.

@ser-0xff
Copy link
Contributor Author

ser-0xff commented Mar 27, 2024

Hi, @Lukasa
Thank you for quick review.
We develop a framework trying to use most available possibilities to make it scalable, including swift concurrency.
While swift-nio is mostly synchronous now all data we get from sockets we propagate to AsyncStreams as ByteBuffer. Data from AsyncStreams is processed with swift Task, so from that point we are in swift async world. The code supposed to process data from the ByteBuffer is async as well, thus we can't use a synchronous variant of withUnsafeReadableBytes.
So the idea is to write a code like:

let stream = AsyncStream<ByteBuffer>() {...}
Task {
    for await byteBuffer in stream {
        try await byteBuffer.withUnsafeReadableBytes { bytes in
             try await processData(bytes)
        }
    }
}

@Lukasa
Copy link
Contributor

Lukasa commented Mar 27, 2024

That makes total sense: I think my core question is just why you need an unsafe pointer through an async boundary. Is it possible to use something safer at that layer?

@ser-0xff
Copy link
Contributor Author

ser-0xff commented Mar 27, 2024

We could propagate ByteBuffer further to the data processing layer, but it is different package where we would not like to have a dependency on swift-nio.
And even if we would have a dependency and propagate ByteBuffer then sooner or later we will need to process data from that ByteBuffer and we will be limited with a synchronous closure still. We even couldn't call any actor inside that closure.

Anyway, if you think it is dangerous let's close PR then.

@hassila
Copy link
Contributor

hassila commented Apr 25, 2024

Maybe the longer term would be the adoption of BufferView (https://gist.github.com/atrick/4fab6886518f756295f77445e4bf0788) when it happens to avoid depending on NIO for ByteBuffer?

Have there been any thoughts on how that would dovetail with current ByteBuffer usage in SwiftNIO?

@Lukasa
Copy link
Contributor

Lukasa commented Apr 25, 2024

So using something like BufferView would be the ideal long-term solution. We have appropriate APIs on ByteBuffer to create one of these for yourself. Specifically you can use withUnsafeReadableBytesWithStorageManagement. This is designed to explicitly allow you to escape the pointer. You can therefore write a wrapper type around this that re-adds memory safety on top of it.

In the longer term, we'll adopt BufferView variants of most of ByteBuffer's with APIs.

@ser-0xff
Copy link
Contributor Author

Thank you for the update!
Will try that approach then.

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