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

Please add ReadOnlySequence<byte> Overloads to Socket.SendAsync #27486

Closed
AceHack opened this issue Sep 26, 2018 · 9 comments
Closed

Please add ReadOnlySequence<byte> Overloads to Socket.SendAsync #27486

AceHack opened this issue Sep 26, 2018 · 9 comments
Assignees
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Sockets
Milestone

Comments

@AceHack
Copy link

AceHack commented Sep 26, 2018

System.Net.Sockets.Socket
{
    public ValueTask<int> SendAsync(ReadOnlySequence<byte> buffer, SocketFlags socketFlags, CancellationToken cancellationToken = default(CancellationToken))
}

See this gist for example usage.

https://gist.github.com/AceHack/786e558f14b2d9c0d46a1ea330c158f7

@davidfowl
Copy link
Member

Agreed we should go through and look at doing this for the most common scenaros in 3.0.

cc @KrzysztofCwalina @ahsonkhan @stephentoub

@BrennanConroy
Copy link
Member

BitConverter.ToString as well

@tarekgh
Copy link
Member

tarekgh commented Jan 17, 2019

@AceHack would you create a complete proposal? you may have a look at https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md for more info.

@davidfowl
Copy link
Member

@tarekgh I can do the proposal but I have a question should I make a proposal per new API or should there be an uber proposal like the Span version of this. I don't think we'll need support for ROS in all the places we have span support but there's more than one.

@tarekgh
Copy link
Member

tarekgh commented Jan 17, 2019

I don't think we need to support ROS in all places support span. I was talking only about the proposed 2 APIs in this issue for Socket and Encoding per the required scenarios. I believe if ROS is needed in any other place, we should evaluate case by case. let me know if you disagree with that.

@davidfowl davidfowl self-assigned this Jan 18, 2019
@GrabYourPitchforks
Copy link
Member

I removed the Encoding extension method API request from this issue because it's now being tracked by https://github.com/dotnet/corefx/issues/41166.

@karelz
Copy link
Member

karelz commented Dec 12, 2019

Triage:
In general we are ok with it.
We should consider also Receive and all other APIs (SendTo, SendtoAsync, etc., etc.).
We should double check that there is perf benefit.

@scalablecory floats around idea to use Span<Memory<byte>> that would work also for smaller buffers.

@davidfowl do you plan to work on the proposal and prototype to confirm perf benefit (e.g. on Kestrel some benchmark)?

@davidfowl
Copy link
Member

@scalablecory floats around idea to use Span<Memory> that would work also for smaller buffers.

How would that work? It's not like you could stackalloc. Also this method is async so the implementation would need to synchronously copy to something on the heap just in case the call goes async. What am I missing?

@davidfowl do you plan to work on the proposal and prototype to confirm perf benefit (e.g. on Kestrel some benchmark)?

What do you mean here? Would I add the API and test it out in Kestrel?

@GrabYourPitchforks GrabYourPitchforks changed the title Please add ReadOnlySequence<byte> Overloads Please add ReadOnlySequence<byte> Overloads to Socket.SendAsync Jan 10, 2020
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@karelz karelz modified the milestones: 5.0, Future Jun 5, 2020
@davidfowl
Copy link
Member

We can replace this with #49941. We'd need this other API anyways since we'd need to transform the ReadOnlySequence into a List<ReadOnlyMemory<byte>> anyways. Once we get a new socket API then we can add a helper to transform the ROS

@ghost ghost locked as resolved and limited conversation to collaborators Apr 22, 2021
@karelz karelz modified the milestones: Future, 6.0.0 May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Sockets
Projects
None yet
Development

No branches or pull requests

7 participants