-
Notifications
You must be signed in to change notification settings - Fork 590
.NET Core port - DO NOT MERGE #175
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
Conversation
Slick work. |
Thanks!! I'll be needing this port soon, so thank you very much! |
+1 DotNetCore/Kestral version of my server dropped to 5k RPS... from an easy 50k. |
The dependency NETStandard.Library >= 1.6.0 could not be resolved. Fleck G:\Drive\Avarice\SignalMQ\Fleck-master\core\Fleck\project.json 5 |
After some fiddling, I got the project down to 4 errors with the following ` "dependencies": { "frameworks": { "buildOptions": { The errors being the removal of the BeginAsync/EndAsync and missing System.Security.SSLStream Do we have an ETA when this will be ready for testing. I recently got the microsoft websocket up to 800,000 OPS would like to test out fleck again. @darkl |
@NVentimiglia, I worked on this a bit today. This works fine on my machine with .NET Core RTM:
Tests don't compile since a .NET Core compatible Moq isn't available yet. I'm not sure if I'll finish this, but you're welcome to continue from where I've stopped. Elad |
7/9/2016 12:31:43 PM [Info] Server started at ws://0.0.0.0:5000 (actual port 5000) |
Are you using the latest version? (netcore branch) Elad |
|
||
public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) | ||
{ | ||
return _stream.WriteAsync(buffer, offset, count, cancellationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be awaited ?
|
||
public override Task FlushAsync(CancellationToken cancellationToken) | ||
{ | ||
return _stream.FlushAsync(cancellationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be awaited ?
|
||
public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) | ||
{ | ||
return _stream.CopyToAsync(destination, bufferSize, cancellationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be awaited ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awaiting would just wrap the CopyToAsync task with another task that does nothing but return the original result. Why do you think it should be wrapped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never really thought of it that way. You are right.
Not needed anymore. |
Hi,
I tried for fun to make this library be .NET Core compatible.
I made a project.json file and made it compile, note that I changed some things: there is no such thing "InvariantCultureIgnoreCase", so I replaced it with "OrdinalIgnoreCase",
there are no more "Close" methods for streams and sockets, so I replaced it with "Dispose".
I also didn't reimplement QueuedStream since I'm not sure if this hack is needed for .NET Core. I also didn't covert the tests.
I didn't try this, I don't know if it works.
I'm opening this pull request, just for the case that anyone wants to continue this work on the port. Please don't merge it until tests are converted, pass, and until this is actually tested.
Best regards,
Elad