Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Added Span overloads for Socket.SendFile#47230

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
antonfirsov merged 5 commits intodotnet:masterfromgfoidl:socket-sendfile-span
Jan 25, 2021

Conversation

@gfoidl
Copy link
Member

Fixes#43846

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

@ghost
Copy link

Note regarding thenew-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

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:-

@geoffkizer
Copy link
Contributor

Tests?

@gfoidl
Copy link
MemberAuthor

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.

gfoidl reacted with thumbs up emoji

@antonfirsov
Copy link
Contributor

antonfirsov commentedJan 21, 2021•
edited
Loading

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

I strongly suggest to do it in this PR.

@gfoidl
Copy link
MemberAuthor

@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 withSocketTestHelperBase<T> and how the things play together, especially as

publicclassSendFileTest: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 theSocketTestHelperBase<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
Contributor

antonfirsov commentedJan 21, 2021•
edited
Loading

@gfoidlFileCleanupTestBase.GetTestFilePath() is used to create and auto-cleanup temporary files. TheTempFile 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 currentSendFile.cs further by coding the same tests for the newSpan<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 reacted with thumbs up emoji

@gfoidl
Copy link
MemberAuthor

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 theSocketTestHelperBase<T> work until after we merge this PR, but we should do it before we add the Task-based SendFile overloads.

gfoidl reacted with thumbs up emoji

@geoffkizer
Copy link
Contributor

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

@gfoidl
Copy link
MemberAuthor

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

@antonfirsov
Copy link
Contributor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@antonfirsovantonfirsov 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.

}
}

[ConditionalTheory]
Copy link
Contributor

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 evaluatebool members at runtime.

gfoidl reacted with thumbs up emoji

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

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 checkSocketException.SocketErrorCode?

/// <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
Contributor

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 theFileNotFoundException case.

Copy link
MemberAuthor

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
Contributor

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.

@antonfirsov
Copy link
Contributor

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

@antonfirsovantonfirsov merged commita473a9c intodotnet:masterJan 25, 2021
@gfoidlgfoidl deleted the socket-sendfile-span branchJanuary 25, 2021 12:30
@antonfirsov
Copy link
Contributor

@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 reacted with hooray emoji

@gfoidl
Copy link
MemberAuthor

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

@karelzkarelz added this to the6.0.0 milestoneJan 26, 2021
antonfirsov added a commit that referenced this pull requestJan 27, 2021
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.
@ghostghost locked asresolvedand limited conversation to collaboratorsFeb 27, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@stephentoubstephentoubstephentoub approved these changes

+2 more reviewers

@antonfirsovantonfirsovantonfirsov approved these changes

@geoffkizergeoffkizergeoffkizer left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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
[8]ページ先頭
©2009-2025 Movatter.jp