Skip to content
This repository was archived by the owner on Aug 2, 2023. It is now read-only.

Fix RIO Connection Receives #1486

Closed
wants to merge 10 commits into from
Closed

Conversation

KodrAus
Copy link
Contributor

@KodrAus KodrAus commented Apr 20, 2017

Rebase of #1420

Fixes an issue with RioTcpConnections not flushing receives.

cc: @davidfowl @benaadams

Changes:

  • Remove the offloading of notifies, there isn't a separate thread for them anymore
  • Complete receives synchronously, rather than flushing later
  • Ensure the RioThread is in a valid state before from ctor
  • Add a stress test for RioTcpServer

@KodrAus
Copy link
Contributor Author

KodrAus commented Apr 20, 2017

Hm, looks like release build is hanging. I'll investigate locally.

@KodrAus
Copy link
Contributor Author

KodrAus commented Apr 21, 2017

Trying out an idea to fix the build hanging. If it works, I'll clean up then ping for review.

@KodrAus
Copy link
Contributor Author

KodrAus commented Apr 21, 2017

r? @davidfowl

Build is green, but I'm not totally confident about it. I think it needs a few more runs.

@ahsonkhan ahsonkhan requested a review from davidfowl April 27, 2017 21:00
@ahsonkhan
Copy link
Contributor

Build is green, but I'm not totally confident about it. I think it needs a few more runs.

@KodrAus, any updates to this PR?

@KodrAus
Copy link
Contributor Author

KodrAus commented Apr 27, 2017

@ahsonkhan I can't reproduce the flakiness locally so have put a Skip on the test, which I suspect is the issue anyways.

I still think it's worth a review because the code before simply didn't work. So assuming the rest is good I guess we can either:

  • Merge as-is with the Skip
  • Remove the test
  • Forget this code and work on porting the Kestrel transport instead

I'm happy with whatever approach.

@ahsonkhan
Copy link
Contributor

I still think it's worth a review because the code before simply didn't work. So assuming the rest is good I guess we can either:
Merge as-is with the Skip
Remove the test
Forget this code and work on porting the Kestrel transport instead
I'm happy with whatever approach.

I will let @davidfowl. @pakrym comment on that.

for (int loop = 0; loop < ClientCount; loop++)
{
using (var thread = new UvThread())
using (var connection = await new UvTcpClient(thread, endpoint).ConnectAsync())
Copy link
Member

Choose a reason for hiding this comment

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

Use a sockets client

@KodrAus
Copy link
Contributor Author

KodrAus commented May 2, 2017

@davidfowl Switched to socket client, which surfaced the many problems with my test. I've fixed those up.

I've made the RIO server a lot slower, but at least we know it works now.

@ahsonkhan
Copy link
Contributor

@davidfowl, comments?

@KodrAus
Copy link
Contributor Author

KodrAus commented May 22, 2017

@davidfowl any more feedback?

@davidfowl
Copy link
Member

Does it work now? @benaadams did you take a look?

@KodrAus
Copy link
Contributor Author

KodrAus commented May 22, 2017

Yep, it totally works now.

@ahsonkhan
Copy link
Contributor

ping @benaadams

@davidfowl, @KodrAus, is this good to go?

@KodrAus
Copy link
Contributor Author

KodrAus commented Jun 7, 2017

@ahsonkhan It's just waiting on a review

@ahsonkhan
Copy link
Contributor

@davidfowl, @benaadams - please take a look

@ahsonkhan
Copy link
Contributor

ahsonkhan commented Sep 22, 2017

@KodrAus, is this change still relevant? If so, can you please fix the merge conflict? Ty.

@davidfowl, @benaadams, @KrzysztofCwalina - are we taking this PR? If not, we should close it.

@KodrAus
Copy link
Contributor Author

KodrAus commented Sep 22, 2017

@ahsonkhan I haven't looked at this in a while. I'll take a look at the conflict, this might not be needed anymore.

@ahsonkhan
Copy link
Contributor

Closing this PR. Please feel free to re-open if necessary.

@ahsonkhan ahsonkhan closed this Sep 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants