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 Memory support to SendPacketElements #45267

Closed
geoffkizer opened this issue Nov 26, 2020 · 9 comments · Fixed by #46975
Closed

Add Memory support to SendPacketElements #45267

geoffkizer opened this issue Nov 26, 2020 · 9 comments · Fixed by #46975
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Sockets
Milestone

Comments

@geoffkizer
Copy link
Contributor

geoffkizer commented Nov 26, 2020

Proposed API

namespace System.Net.Sockets
{
    public class SendPacketsElement
    {
        // Existing APIs:
        public SendPacketsElement(byte[] buffer);
        public SendPacketsElement(byte[] buffer, int offset, int count);
        public SendPacketsElement(byte[] buffer, int offset, int count, bool endOfPacket);

        public byte[]? Buffer { get; }
        public int Offset { get; }
        public int Count { get; }

        // Proposed APIs:
        public SendPacketsElement(ReadOnlyMemory<byte> buffer);
        public SendPacketsElement(ReadOnlyMemory<byte> buffer, bool endOfPacket);

        public ReadOnlyMemory<byte>? MemoryBuffer { get; }
    }
}

This is modeled on how we added Memory support to SocketAsyncEventArgs.

If you use a new Memory constructor overload, then:

  • MemoryBuffer will return the ReadOnlyMemory that you passed to the constructor
  • Buffer will return null
  • Offset will return 0
  • Count will return MemoryBuffer.Length

If you use an old byte[] constructor overload, then:

  • MemoryBuffer will return a ReadOnlyMemory constructed from the byte[], offset, and count you passed to the constructor.
  • Buffer, Offset, and Count will return the values you passed to the constructor (as today)
@geoffkizer geoffkizer added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Sockets labels Nov 26, 2020
@geoffkizer geoffkizer added this to the 6.0.0 milestone Nov 26, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Nov 26, 2020
@ghost
Copy link

ghost commented Nov 26, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Proposed API

namespace System.Net.Sockets
{
    public class SendPacketsElement
    {
        // Existing APIs:
        public SendPacketsElement(byte[] buffer);
        public SendPacketsElement(byte[] buffer, int offset, int count);
        public SendPacketsElement(byte[] buffer, int offset, int count, bool endOfPacket);

        public byte[]? Buffer { get; }
        public int Offset { get; }
        public int Count { get; }

        // Proposed APIs:
        public SendPacketsElement(ReadOnlyMemory<byte> buffer);
        public SendPacketsElement(ReadOnlyMemory<byte> buffer, bool endOfPacket);

        public Memory<byte>? MemoryBuffer { get; }
    }
}

This is modeled on how we added Memory support to SocketAsyncEventArgs.

If you use a new Memory constructor overload, then:

  • MemoryBuffer will return the Memory that you passed to the constructor
  • Buffer will return null
  • Offset will return 0
  • Count will return MemoryBuffer.Length

If you use an old byte[] constructor overload, then:

  • MemoryBuffer will return a Memory constructed from the byte[], offset, and count you passed to the constructor.
  • Buffer, Offset, and Count will return the values you passed to the constructor (as today)
Author: geoffkizer
Assignees: -
Labels:

api-suggestion, area-System.Net.Sockets

Milestone: 6.0.0

@scalablecory
Copy link
Contributor

  • MemoryBuffer will return a ReadOnlyMemory constructed from the byte[], offset, and count you passed to the constructor.

if this is the case, there's no reason for the MemoryBuffer property to be nullable.

@gfoidl
Copy link
Member

gfoidl commented Nov 28, 2020

MemoryBuffer property to be nullable

Besides that it shouldn't be nullable anyway. If not set, it's default, so IsEmpty will return true.
So to work with the check for IsEmpty or HasValue has to be done in either way.

But this is superceded by @scalablecory's comment.

@geoffkizer
Copy link
Contributor Author

if this is the case, there's no reason for the MemoryBuffer property to be nullable.

SendPacketElements can also be a file (or file segment). In that case, MemoryBuffer will return null, as Buffer does today.

@gfoidl
Copy link
Member

gfoidl commented Nov 30, 2020

...or MemoyBuffer will just be empty (instead of being null).

For span-overloads if no data is given, they're empty and we don't have nullable spans as arguments where null means void.
Same for other memory-overloads. I see this case in the same vein.

@geoffkizer
Copy link
Contributor Author

I see this case in the same vein.

I think it's different. In this case you're not using a Memory at all; you are using a file.

@geoffkizer
Copy link
Contributor Author

@gfoidl
Copy link
Member

gfoidl commented Nov 30, 2020

Ah, ok there it is also nullable, so it should be the same here.
You're right.

@geoffkizer geoffkizer added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Jan 5, 2021
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jan 12, 2021
@terrajobst
Copy link
Member

terrajobst commented Jan 12, 2021

  • Looks good as proposed
namespace System.Net.Sockets
{
    public partial class SendPacketsElement
    {
        // Existing APIs:
        // public SendPacketsElement(byte[] buffer);
        // public SendPacketsElement(byte[] buffer, int offset, int count);
        // public SendPacketsElement(byte[] buffer, int offset, int count, bool endOfPacket);
        // public byte[]? Buffer { get; }
        // public int Offset { get; }
        // public int Count { get; }

        public SendPacketsElement(ReadOnlyMemory<byte> buffer);
        public SendPacketsElement(ReadOnlyMemory<byte> buffer, bool endOfPacket);
        public ReadOnlyMemory<byte>? MemoryBuffer { get; }
    }
}

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 14, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 15, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Sockets
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants