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.

Sign up for GitHub

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

Jump to bottom

add Memory<byte> support in SendPacketElements and SendPacketsAsync #46975

Merged
merged 8 commits into from
Jan 15, 2021
Merged

add Memory<byte> support in SendPacketElements and SendPacketsAsync #46975

merged 8 commits into from
Jan 15, 2021

Conversation

geoffkizer
Copy link
Contributor

Fixes #45267

@antonfirsov @stephentoub @dotnet/ncl

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jan 14, 2021

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

Issue Details

Fixes #45267

@antonfirsov @stephentoub @dotnet/ncl

Author: geoffkizer
Assignees: -
Labels:

area-System.Net.Sockets, new-api-needs-documentation
Milestone: -

@geoffkizer
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@gfoidl gfoidl mentioned this pull request Jan 14, 2021
Closed
antonfirsov
antonfirsov reviewed Jan 14, 2021
View reviewed changes
Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

A few concerns regarding tests, otherwise looks good.

I would add at least one test that uses NativeMemoryManager to wrap an actual pointer into ReadOnlyMemory<byte> & apply custom some offset + length there to stress that behavior too.

src/libraries/System.Net.Sockets/tests/FunctionalTests/SendPacketsAsync.cs Outdated Show resolved Hide resolved
src/libraries/System.Net.Sockets/tests/FunctionalTests/SendPacketsAsync.cs Outdated Show resolved Hide resolved
@runfoapp runfoapp bot mentioned this pull request Jan 14, 2021
Closed
@geoffkizer
Copy link
Contributor Author

There are also a bunch of tests marked as [Outerloop] that don't need to be, as far as I can tell. Will remove this.

@geoffkizer
Copy link
Contributor Author

Feedback addressed. Please take a look.

@geoffkizer
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov
antonfirsov approved these changes Jan 15, 2021
View reviewed changes
Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

LGTM

src/libraries/System.Net.Sockets/tests/FunctionalTests/SendPacketsAsync.cs
return new ElementWithMemoryManager(new SendPacketsElement(memoryManager.Memory.Slice(offset, count)), memoryManager);
}

private static ElementWithMemoryManager CreateElementForBuffer(BufferType bufferType, int size) =>
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@geoffkizer geoffkizer merged commit 059b17e into dotnet:master Jan 15, 2021
@geoffkizer geoffkizer deleted the sendpacketelementsmemory branch January 15, 2021 17:57
@geoffkizer geoffkizer mentioned this pull request Jan 15, 2021
Open
14 tasks
@karelz karelz added this to the 6.0.0 milestone Jan 26, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@antonfirsov antonfirsov antonfirsov approved these changes

Assignees
No one assigned
Labels
area-System.Net.Sockets new-api-needs-documentation
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

Add Memory support to SendPacketElements
4 participants
@geoffkizer @stephentoub @antonfirsov @karelz