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

Added Span overloads for Socket.SendFile #47230

Merged
merged 5 commits into from
Jan 25, 2021
Merged

Added Span overloads for Socket.SendFile #47230

merged 5 commits into from
Jan 25, 2021

Conversation

gfoidl
Copy link
Member

@gfoidl gfoidl commented Jan 20, 2021

Fixes #43846

This PR is split-out of #45132 -- see #45132 (comment) for context.

@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 20, 2021

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

Issue Details

Fixes #43846

This PR is split-out of #45132 -- see #45132 (comment) for context.

Author: gfoidl
Assignees: -
Labels:

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

@gfoidl gfoidl mentioned this pull request Jan 20, 2021
Closed
@geoffkizer
Copy link
Contributor

Tests?

@gfoidl
Copy link
Member Author

gfoidl commented Jan 20, 2021

Tests?

Tests for the array-based implementation are there, and as these forward to the span-based implementation that's covered.
Explicit span tests are a duplication, but I can add those if you want.

@geoffkizer
Copy link
Contributor

We should add explicit span tests too. Yeah it's duplicative, but that's fine; better to be duplicative than to miss something.

@antonfirsov
Copy link
Member

antonfirsov commented Jan 21, 2021 •
edited
Loading

To support all the refactoring described in #45132 (comment) and #43845, we should refactor SendFileTest to derive from SocketTestHelperBase<T>, similarly to #46862.

I strongly suggest to do it in this PR.

@gfoidl
Copy link
Member Author

gfoidl commented Jan 21, 2021

@antonfirsov the refactoring of the tests makes sense, but can this be done in a different PR? I'd like to focus this PR on adding the span-overloads.

And maybe the major reason for my pushback is, that I'm not familiar with SocketTestHelperBase<T> and how the things play together, especially as

runtime/src/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs

Line 15 in f066972

public class SendFileTest : FileCleanupTestBase
has already a base-class, so this needs to be moved around and I have no clue on how to do this at the moment.
I see #46862 and other code that uses the SocketTestHelperBase<T> but this doesn't make it sound for that change.
Maybe I'm missing a trivial piece of the puzzle now, or we have to do this separately.

@antonfirsov
Copy link
Member

antonfirsov commented Jan 21, 2021 •
edited
Loading

@gfoidl FileCleanupTestBase.GetTestFilePath() is used to create and auto-cleanup temporary files. The TempFile utility is an equivalent tool for this, and it doesn't need subclassing:
https://github.com/dotnet/runtime/blob/master/src/libraries/Common/tests/System/IO/TempFile.cs

SocketTestHelperBase<T> is a utility to avoid over-duplicating the same test logic across all the Sync / APM (Begin, End) / SocketAsyncEventArgs / Task test variants. This makes it easier to ensure that the cases which are valid for all those variants are indeed tested for all of them. Using it usually results in better coverage.

What I wanted was to avoid bloating the current SendFile.cs further by coding the same tests for the new Span<byte> overloads (just to delete them a few weeks later). Nevertheless, if a separate PR is easier for you, I'm fine with that, and I may also take that work after finishing #47229.

@gfoidl
Copy link
Member Author

gfoidl commented Jan 21, 2021

Thanks for the explaination!
Please let's sheperd this PR to merge, then do the test stuff where I'd really appreciate

I'm fine with that, and I may also take that work after finishing #47229.

to see how it's done (and use that knowledge then for the SendFileAsync tests).

@geoffkizer
Copy link
Contributor

I'm fine to defer the SocketTestHelperBase<T> work until after we merge this PR, but we should do it before we add the Task-based SendFile overloads.

@geoffkizer
Copy link
Contributor

ok -- are we ready to merge then? @antonfirsov any remaining concerns?

@gfoidl
Copy link
Member Author

gfoidl commented Jan 21, 2021

I have no more changes planned for this PR. So ready to merge from my side.

@geoffkizer geoffkizer mentioned this pull request Jan 21, 2021
Open
@antonfirsov
Copy link
Member

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov
antonfirsov approved these changes Jan 22, 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. I have some comments regarding tests, but let's ignore them, since it doesn't make sense to do alterations to test code we are about to completely refactor anyways.

I want to merge as soon as the outerloop run finishes.

src/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs
Assert.Throws<NotSupportedException>(() => s.BeginSendFile(null, null, null));
Assert.Throws<NotSupportedException>(() => s.BeginSendFile(null, null, null, TransmitFileOptions.UseDefaultWorkerThread, null, null));
}
}

[ConditionalTheory]
Copy link
Member

Choose a reason for hiding this comment

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

[PlatformSpecific] is enough to make this conditional, [ConditionalTheory] is used when you need to evaluate bool members at runtime.

src/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs

if (useAsync)
{
await Assert.ThrowsAsync<SocketException>(() => Task.Factory.FromAsync<string>(client.BeginSendFile, client.EndSendFile, filename, null));
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make sense to also check SocketException.SocketErrorCode?

src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
/// <exception cref="ObjectDisposedException">The <see cref="Socket"/> object has been closed.</exception>
/// <exception cref="NotSupportedException">The <see cref="Socket"/> object is not connected to a remote host.</exception>
/// <exception cref="InvalidOperationException">The <see cref="Socket"/> object is not in blocking mode and cannot accept this synchronous call.</exception>
/// <exception cref="FileNotFoundException">The file <paramref name="fileName"/> was not found.</exception>
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we don't have test for the FileNotFoundException case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, that's missing.

As the tests will be refactored, should we file an issue for this and where these points get collected? Or won't we forget about this anyway?

Copy link
Member

Choose a reason for hiding this comment

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

If one of us manages to PR the test refactor within a week, I wouldn't bother with an issue, otherwise it would be wise to start tracking it.

My main fear is that more extensive testing will discover a bunch of corner cases and platform inconsistencies (or even actual bugs) we'll have to deal with. This is what made things hard with my other PR.

This was referenced Jan 25, 2021
Open
Closed
@antonfirsov
Copy link
Member

OuterLoop test failures are unrelated:
#41606
#1724
#21631 (comment)
#47103
#46439 (test run before merging the fix)

@antonfirsov antonfirsov merged commit a473a9c into dotnet:master Jan 25, 2021
@gfoidl gfoidl deleted the socket-sendfile-span branch January 25, 2021 12:30
@antonfirsov
Copy link
Member

@gfoidl FYI I have started refactoring the SendFile tests, if everything goes well, expect a PR within a day.
https://github.com/dotnet/runtime/compare/master...antonfirsov:sockets/SendFile-test-refactor?expand=1

@gfoidl
Copy link
Member Author

gfoidl commented Jan 26, 2021

Thanks for the update -- happy to have a look at the PR (then start the async versions of sendfile).

@karelz karelz added this to the 6.0.0 milestone Jan 26, 2021
@antonfirsov antonfirsov mentioned this pull request Jan 26, 2021
Merged
antonfirsov added a commit that referenced this pull request Jan 27, 2021
@antonfirsov
Refactor Socket.SendFile tests and extend coverage (#47479)
0ff4f4a 
Refactor tests in SendFile.cs to utilize SocketTestHelper<T>
As a result cover some cases which were uncovered before (for example Dispose VS async).
Add more checks and tests as discussed in #47230.
@antonfirsov antonfirsov mentioned this pull request Jan 29, 2021
Merged
@antonfirsov antonfirsov mentioned this pull request Feb 18, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Feb 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@geoffkizer geoffkizer geoffkizer left review comments

@antonfirsov antonfirsov antonfirsov approved these changes

@stephentoub stephentoub stephentoub 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 Span overloads for Socket.SendFile
5 participants
@gfoidl @geoffkizer @antonfirsov @stephentoub @karelz