Extract meta data about open sockets into separate class.

Extract meta data about open sockets into separate class.

The current implementation only has information about tcp and udp
sockets, and we explicitly do not track internal and pipe uses of
_NativeSockets.

The testing of this is done through the service layer, which is the end user of
this.

BUG=
R=sgjesse@google.com

Committed: https://github.com/dart-lang/sdk/commit/01eff8ce18be7eceb6ce0be1097005965edf2588

Committed: https://github.com/dart-lang/sdk/commit/06ab2bdbdda2ad1696a842ea44d8fd0d38106f7f

[ricow1, 2015-09-01 12:06:24]

ricow@google.com changed reviewers:
+ sgjesse@google.com

[ricow1, 2015-09-01 12:06:25]

I don't know yet if this is the actual structure that John wants the data in,
but I would like some feedback on if this is completely off.

https://codereview.chromium.org/1323943003/diff/1/runtime/bin/socket_patch.dart
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/1323943003/diff/1/runtime/bin/socket_patch.da...
runtime/bin/socket_patch.dart:578: if (!isInternal && !isPipe) {
These "if (!isInternal && !isPipe) {" will go away when I add support for pipes
and internal uses. The fields used is already in the superclass

https://codereview.chromium.org/1323943003/diff/1/runtime/bin/socket_patch.da...
runtime/bin/socket_patch.dart:926: if (!connectedResourceHandler) {
I am not sure this is the correct location for this, if you have a better place
let me know.

[Søren Gjesse, 2015-09-02 09:07:50]

lgtm, with comments

https://codereview.chromium.org/1323943003/diff/1/runtime/bin/socket_patch.dart
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/1323943003/diff/1/runtime/bin/socket_patch.da...
runtime/bin/socket_patch.dart:437: setupResourceInfo(socket);
Shouldn't this move down to just after timer.cancel() below?

What is happening with connectNext is that if a destination have more than one
IP address as the result of a DNS lookup each of these are tired with a timeout,
and the first that actually reports as being connected will close any other open
ones in the "connecting" Map.

https://codereview.chromium.org/1323943003/diff/1/runtime/bin/socket_patch.da...
runtime/bin/socket_patch.dart:578: if (!isInternal && !isPipe) {
On 2015/09/01 12:06:24, ricow1 wrote:
> These "if (!isInternal && !isPipe) {" will go away when I add support for
pipes
> and internal uses. The fields used is already in the superclass

Please add a TODO then.

Maybe check if resourceInfo is non-null instead (in the other places as well).

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
File sdk/lib/io/io_resource_info.dart (right):

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
sdk/lib/io/io_resource_info.dart:8: String type;
Can't type and id and be final?

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
sdk/lib/io/io_resource_info.dart:23: Map<String, String> get referenceValueMap
=>
Cache this value?

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
sdk/lib/io/io_resource_info.dart:25: 'type': '@$type',
What is the reason for the @?

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
sdk/lib/io/io_resource_info.dart:68: class _SocketResourceInfo extends
_ReadWriteResourceInfo{
nit: missing space before {.

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
sdk/lib/io/io_resource_info.dart:78: String get name {
Cache the String generated here?

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
sdk/lib/io/io_resource_info.dart:97: result['socket_type'] = socket.isTcp ?
'TCP' : 'UDP';
Please make constants for these string keys.

[ricow1, 2015-09-02 10:45:42]

More questions than answers :-) PTAL

https://codereview.chromium.org/1323943003/diff/1/runtime/bin/socket_patch.dart
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/1323943003/diff/1/runtime/bin/socket_patch.da...
runtime/bin/socket_patch.dart:437: setupResourceInfo(socket);
On 2015/09/02 09:07:49, Søren Gjesse wrote:
> Shouldn't this move down to just after timer.cancel() below?
> 
> What is happening with connectNext is that if a destination have more than one
> IP address as the result of a DNS lookup each of these are tired with a
timeout,
> and the first that actually reports as being connected will close any other
open
> ones in the "connecting" Map.

If I do that, and there is an error, I do a socket.close(), which will trigger
the resource info to be removed (but we can't remove it, because it is not
there)

https://codereview.chromium.org/1323943003/diff/1/runtime/bin/socket_patch.da...
runtime/bin/socket_patch.dart:578: if (!isInternal && !isPipe) {
On 2015/09/02 09:07:49, Søren Gjesse wrote:
> On 2015/09/01 12:06:24, ricow1 wrote:
> > These "if (!isInternal && !isPipe) {" will go away when I add support for
> pipes
> > and internal uses. The fields used is already in the superclass
> 
> Please add a TODO then.
> 
> Maybe check if resourceInfo is non-null instead (in the other places as well).

So the reason I did not want to do that is that I want to actually catch cases
where we did not setup resouceInfo, but where not internal or pipe (i.e., we
could get into a situation where we had an open socket without a resourceInfo
object, which we then would not see in the observatory). I can change this to:
if (resourceInfo != null) {
  resourceInfo.readCount++;
  resourceInfo.lastRead = timestamp;
} else {
  assert(isInternal or isPipe);
}
but that is more verbose. I have no strong feelings towards one or the other,
but would like to at least keep the coverage, I will let you decide. Added TODO
here and the other places.

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
File sdk/lib/io/io_resource_info.dart (right):

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
sdk/lib/io/io_resource_info.dart:8: String type;
On 2015/09/02 09:07:49, Søren Gjesse wrote:
> Can't type and id and be final?

Done.

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
sdk/lib/io/io_resource_info.dart:23: Map<String, String> get referenceValueMap
=>
On 2015/09/02 09:07:49, Søren Gjesse wrote:
> Cache this value?

Imagine how few times this will be called compared to the number of sockets
created. This will only be called (on all open sockets), if somebody presses the
sockets link in the observatory. If we cache it we waste an extra field for
every socket object ever created. I don't really have strong feelings on this
though, and can add caching.

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
sdk/lib/io/io_resource_info.dart:25: 'type': '@$type',
On 2015/09/02 09:07:49, Søren Gjesse wrote:
> What is the reason for the @?

John told me that the reference objects have that in front,

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
sdk/lib/io/io_resource_info.dart:68: class _SocketResourceInfo extends
_ReadWriteResourceInfo{
On 2015/09/02 09:07:49, Søren Gjesse wrote:
> nit: missing space before {.

Done.

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
sdk/lib/io/io_resource_info.dart:78: String get name {
On 2015/09/02 09:07:49, Søren Gjesse wrote:
> Cache the String generated here?

Same answer as above, happy to add caching though if you still want it

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
sdk/lib/io/io_resource_info.dart:97: result['socket_type'] = socket.isTcp ?
'TCP' : 'UDP';
On 2015/09/02 09:07:49, Søren Gjesse wrote:
> Please make constants for these string keys.

Done.

[Søren Gjesse, 2015-09-02 12:24:50]

lgtm

https://codereview.chromium.org/1323943003/diff/1/runtime/bin/socket_patch.dart
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/1323943003/diff/1/runtime/bin/socket_patch.da...
runtime/bin/socket_patch.dart:578: if (!isInternal && !isPipe) {
On 2015/09/02 10:45:42, ricow1 wrote:
> On 2015/09/02 09:07:49, Søren Gjesse wrote:
> > On 2015/09/01 12:06:24, ricow1 wrote:
> > > These "if (!isInternal && !isPipe) {" will go away when I add support for
> > pipes
> > > and internal uses. The fields used is already in the superclass
> > 
> > Please add a TODO then.
> > 
> > Maybe check if resourceInfo is non-null instead (in the other places as
well).
> 
> So the reason I did not want to do that is that I want to actually catch cases
> where we did not setup resouceInfo, but where not internal or pipe (i.e., we
> could get into a situation where we had an open socket without a resourceInfo
> object, which we then would not see in the observatory). I can change this to:
> if (resourceInfo != null) {
>   resourceInfo.readCount++;
>   resourceInfo.lastRead = timestamp;
> } else {
>   assert(isInternal or isPipe);
> }
> but that is more verbose. I have no strong feelings towards one or the other,
> but would like to at least keep the coverage, I will let you decide. Added
TODO
> here and the other places.

I like the more verbose version with the assert.

https://codereview.chromium.org/1323943003/diff/1/runtime/bin/socket_patch.da...
runtime/bin/socket_patch.dart:926: if (!connectedResourceHandler) {
On 2015/09/01 12:06:25, ricow1 wrote:
> I am not sure this is the correct location for this, if you have a better
place
> let me know.

I think this is fine for now.

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
File sdk/lib/io/io_resource_info.dart (right):

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
sdk/lib/io/io_resource_info.dart:23: Map<String, String> get referenceValueMap
=>
On 2015/09/02 10:45:42, ricow1 wrote:
> On 2015/09/02 09:07:49, Søren Gjesse wrote:
> > Cache this value?
> 
> Imagine how few times this will be called compared to the number of sockets
> created. This will only be called (on all open sockets), if somebody presses
the
> sockets link in the observatory. If we cache it we waste an extra field for
> every socket object ever created. I don't really have strong feelings on this
> though, and can add caching.

Good point. Lets leave it as is.

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
sdk/lib/io/io_resource_info.dart:25: 'type': '@$type',
On 2015/09/02 10:45:42, ricow1 wrote:
> On 2015/09/02 09:07:49, Søren Gjesse wrote:
> > What is the reason for the @?
> 
> John told me that the reference objects have that in front, 

Thought so :-) - maybe add a comment.

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
sdk/lib/io/io_resource_info.dart:78: String get name {
On 2015/09/02 10:45:42, ricow1 wrote:
> On 2015/09/02 09:07:49, Søren Gjesse wrote:
> > Cache the String generated here?
> 
> Same answer as above, happy to add caching though if you still want it

ditto answer.

[ricow1, 2015-09-02 12:45:50]

Thank you

https://codereview.chromium.org/1323943003/diff/1/runtime/bin/socket_patch.dart
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/1323943003/diff/1/runtime/bin/socket_patch.da...
runtime/bin/socket_patch.dart:578: if (!isInternal && !isPipe) {
On 2015/09/02 12:24:49, Søren Gjesse wrote:
> On 2015/09/02 10:45:42, ricow1 wrote:
> > On 2015/09/02 09:07:49, Søren Gjesse wrote:
> > > On 2015/09/01 12:06:24, ricow1 wrote:
> > > > These "if (!isInternal && !isPipe) {" will go away when I add support
for
> > > pipes
> > > > and internal uses. The fields used is already in the superclass
> > > 
> > > Please add a TODO then.
> > > 
> > > Maybe check if resourceInfo is non-null instead (in the other places as
> well).
> > 
> > So the reason I did not want to do that is that I want to actually catch
cases
> > where we did not setup resouceInfo, but where not internal or pipe (i.e., we
> > could get into a situation where we had an open socket without a
resourceInfo
> > object, which we then would not see in the observatory). I can change this
to:
> > if (resourceInfo != null) {
> >   resourceInfo.readCount++;
> >   resourceInfo.lastRead = timestamp;
> > } else {
> >   assert(isInternal or isPipe);
> > }
> > but that is more verbose. I have no strong feelings towards one or the
other,
> > but would like to at least keep the coverage, I will let you decide. Added
> TODO
> > here and the other places.
> 
> I like the more verbose version with the assert.
Done (instead I put the assert before)

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
File sdk/lib/io/io_resource_info.dart (right):

https://codereview.chromium.org/1323943003/diff/1/sdk/lib/io/io_resource_info...
sdk/lib/io/io_resource_info.dart:25: 'type': '@$type',
On 2015/09/02 12:24:50, Søren Gjesse wrote:
> On 2015/09/02 10:45:42, ricow1 wrote:
> > On 2015/09/02 09:07:49, Søren Gjesse wrote:
> > > What is the reason for the @?
> > 
> > John told me that the reference objects have that in front, 
> 
> Thought so :-) - maybe add a comment.

Done.

[ricow1, 2015-09-02 13:26:45]

Committed patchset #3 (id:40001) manually as
01eff8ce18be7eceb6ce0be1097005965edf2588 (presubmit successful).

[ricow1, 2015-09-03 12:30:23]

PTAL, I am now more lax with regards to number or reads and writes, since we
can't guarantee the number. Also, on windows the timer granularity is not small
enough to make the assumptions we do.

The differences is the diff against patch set 3

https://codereview.chromium.org/1323943003/diff/40001/sdk/lib/io/io_resource_...
File sdk/lib/io/io_resource_info.dart (right):

https://codereview.chromium.org/1323943003/diff/40001/sdk/lib/io/io_resource_...
sdk/lib/io/io_resource_info.dart:140: static Future<ServiceExtensionResponse>
getSocketInfo(function, params) {
this function was not used, it was leftover

[Søren Gjesse, 2015-09-03 12:39:42]

lgtm

https://codereview.chromium.org/1323943003/diff/90001/runtime/observatory/tes...
File runtime/observatory/tests/service/tcp_socket_service_test.dart (right):

https://codereview.chromium.org/1323943003/diff/90001/runtime/observatory/tes...
runtime/observatory/tests/service/tcp_socket_service_test.dart:53: // Stopwatch
resolution on windows makes us sometimes report 0;
Maybe add a helper for this check - repeated below.

[ricow1, 2015-09-03 17:10:51]

https://codereview.chromium.org/1323943003/diff/90001/runtime/observatory/tes...
File runtime/observatory/tests/service/tcp_socket_service_test.dart (right):

https://codereview.chromium.org/1323943003/diff/90001/runtime/observatory/tes...
runtime/observatory/tests/service/tcp_socket_service_test.dart:53: // Stopwatch
resolution on windows makes us sometimes report 0;
On 2015/09/03 12:39:42, Søren Gjesse wrote:
> Maybe add a helper for this check - repeated below.

Done.

[ricow1, 2015-09-03 17:18:51]

Committed patchset #7 (id:110001) manually as
06ab2bdbdda2ad1696a842ea44d8fd0d38106f7f (presubmit successful).