This repository was archived by the owner on Dec 18, 2018. It is now read-only.
Record remote IPAddress and port #284
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -0,0 +1,14 @@ | |||
| { | |||
There was a problem hiding this comment.
I'll remove this file.
| bytes[i + 8] = bytes2[i]; | ||
| } | ||
|
|
||
| address = new IPAddress(bytes); |
There was a problem hiding this comment.
Right now I assume the IPAddress performs well. If we detect later on it's a bottleneck we can replace it with our own bit-wise operation.
There was a problem hiding this comment.
If this only happens per connection, not per request, then it won't matter as much.
|
Tests? Unit or functional? |
|
I'll put up a few tests. But I'm wondering if a real case scenario can be automated on single machine. |
|
Thanks for doing this. I want to test it but suddenly roslyn decided that it won't compile kestrel at all on my machine. |
| private long _field1; | ||
| private long _field2; | ||
| private long _field3; | ||
| public long _field0; |
|
Have you tried this change out on Mac and/or Linux? |
|
I wanted to but Roslyn is hiccuping on me. It did not want to compile kestrel (due to an exception and not due to errors). I will see if there is a newer dnx and try to compile. I am using Linux x86 |
|
I tried compiling your PR but roslyn is not cooperating: The short enum comparer seems to be part of rolsyn and not the framework. |
|
There a few more changes I'm going on in next iteration. And then I'll test this out on *nix. |
|
Added code to deal with situation when server is not set up as dual-stack sockets. |
| httpConnectionFeature.RemotePort = _remotePort; | ||
| httpConnectionFeature.LocalIpAddress = _localIPAddress; | ||
| httpConnectionFeature.LocalPort = _localPort; | ||
| httpConnectionFeature.IsLocal = IPAddress.IsLoopback(_remoteIPAddress); |
There was a problem hiding this comment.
You don't need to redo this check on each request.
There was a problem hiding this comment.
We've done this check differently elsehwere: https://github.com/aspnet/WebListener/blob/cecd42dfcd624c322be43e4158163b5ac3ec4541/src/Microsoft.Net.Http.Server/RequestProcessing/Request.cs#L301
There was a problem hiding this comment.
We can just store these values somewhere private and reset them in Frame.Reset.
troydai
force-pushed
the
trdai/67
branch
5 times, most recently
from
6ec4233 to
819a2c2
Compare
| }).Build(); | ||
|
|
||
| var builder = new WebHostBuilder(config).UseServer("Microsoft.AspNet.Server.Kestrel") | ||
| .UseStartup(app => |
There was a problem hiding this comment.
This indenting is ridiculous please fix it.
|
Ping |
|
P/Invoke looks good. |
| @@ -99,6 +115,26 @@ public void Reset() | |||
| ResponseBody = null; | |||
| DuplexStream = null; | |||
|
|
|||
| var httpConnectionFeature = this as IHttpConnectionFeature; | |||
| if (httpConnectionFeature != null) | |||
There was a problem hiding this comment.
this cast will never return null
|
Ping? |
| return false; | ||
| } | ||
|
|
||
| return (_field2 & 0xFFFFFFFF) == 0xFFFF0000; |
There was a problem hiding this comment.
return _field1 == 0 && (_field2 & 0xFFFFFFFF) == 0xFFFF0000;
|
|
|
Read to merge but wait for a healthy CI window. |
| } | ||
| else if (IsIPv4MappedToIPv6()) | ||
| { | ||
| var ipv4bits = (_field2 >> 32) & 0x00000000FFFFFFFF; |
There was a problem hiding this comment.
0x00000000FFFFFFFF that's a long literal... I first glance I thought that this was a different mask that what is used on _field0 above, but it's not.
There was a problem hiding this comment.
Why not just get rid of the mask (and the other one as well)? It's a no-op after all.
Or uint.MaxValue
|
🚢 |
|
|
||
| // Quick calculate the port by mask the field and locate the byte 3 and byte 4 | ||
| // and then shift them to correct place to form a int. | ||
| var port = ((int)(_field0 & 0x00FF0000) >> 8) | (int)((_field0 & 0xFF000000) >> 24); |
There was a problem hiding this comment.
Shouldn't the 8 be a 16?
Personally, I prefer shifting first and then using a short mask for readability: (_field0 >> 16) & 0xff
There was a problem hiding this comment.
No, it's correct… My bad
There was a problem hiding this comment.
I like shifting first for readability though.
Provide RemoteIPAddress as well as RemotePort
|
I confirm that this feature works as expected. I am giving up on Nowin and switching fully to Kestrel. |
|
👍 Thanks for trying this out, @borgdylan. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #67
/cc @Tratcher @halter73 @muratg