Skip to content
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

Normal socket wrongly named WebSocket #3033

Closed
julemand101 opened this issue May 15, 2021 · 14 comments · Fixed by #8061
Closed

Normal socket wrongly named WebSocket #3033

julemand101 opened this issue May 15, 2021 · 14 comments · Fixed by #8061
Labels
bug Something isn't working fix it friday P1 high priority issues at the top of the work list, actively being worked on. screen: network Issues with the Network screen.

Comments

@julemand101
Copy link

julemand101 commented May 15, 2021

Just helped another user who got rather confused about seeing "WebSocket" traffic when just making normal HTTP calls. This issue is therefore created in hope of making DevTools less confusing when using it for monitoring network traffic.

The following program will repeatable make use of a fresh HttpClient to make a HTTP request:

import 'dart:io';

Future<void> main(List<String> args) async {
  // ignore: literal_only_boolean_expressions
  while (true) {
    final client = HttpClient();
    await Future<void>.delayed(const Duration(seconds: 10));
    final req = await client.getUrl(Uri.parse('https://example.com/'));
    await req.close();
    client.close();
  }
}

In the "Network" overview we will see the following:
image

The problem is that we are not making any WebSocket connections. Instead, these events comes from normal socket events send by the Dart VM when the HttpClient are preparing the raw socket connection to the web server (later used for the HTTP communication). These events are of the type SocketStatistic and documented here:
https://pub.dev/documentation/vm_service/latest/vm_service/SocketStatistic-class.html

(The reason why the HTTP event comes first is properly because it is sorted by the start time of the event. Since the event contains the time it takes for creating the socket, the HTTP statistic object is properly created before even attempting to create a socket).

Since WebSocket has a very specific meaning in the context of web communication, it is not correct to use the term for normal sockets (or the type "ws"). In fact, we will also see "WebSocket" when just creating a simple server communicating over normal sockets without any use of HTTP:

import 'dart:convert';
import 'dart:io';

Future<void> main(List<String> args) async {
  final serverSocket = await ServerSocket.bind('localhost', 1337);

  serverSocket.listen((socket) async {
    print('Received: ${utf8.decode(await socket.single)}');
    await socket.close();
  });

  // ignore: literal_only_boolean_expressions
  while (true) {
    await Future<void>.delayed(const Duration(seconds: 10));
    final socket = await Socket.connect('localhost', 1337);
    socket.writeln('Hello World');
    await socket.flush();
    await socket.close();
  }
}

image

So in short, I think "WebSocket" should be renamed to "Socket" in the "Network" tab. Also, it would properly make sense if you can toggle if you want to see the sockets in your overview since it is really not that interesting for a lot of people if they just need to know which HTTP request are being sent.

@marcpicaud
Copy link

It is indeed very confusing, especially when trying to debug websocket traffic.

@kenzieschmoll kenzieschmoll added the screen: network Issues with the Network screen. label May 24, 2022
@kenzieschmoll kenzieschmoll removed this from the Backlog milestone May 24, 2022
@lookevink
Copy link

This confusing naming convention remains as of Oct 7, 2022

@kenzieschmoll kenzieschmoll added P3 issues we think are valid but not important and removed P5 labels Jul 5, 2023
@escamoteur
Copy link

Wow, this was opened three years ago and no comment or fix. I observe a lot of them even while we are reusing he same HttpClient

@brianquinlan
Copy link

Would it make sense to make this a P2?

@kenzieschmoll
Copy link
Member

@bkonyi and @brianquinlan the network traffic in question comes from the VmService's getSocketProfile RPC. What are your thoughts WRT to how this data should actually be classified if it is not web socket traffic? Do you have a proposed solution to this issue?

@kenzieschmoll kenzieschmoll added P2 important to work on, but not at the top of the work list. and removed P3 issues we think are valid but not important labels Jul 9, 2024
@escamoteur
Copy link

I'm not sure when this was changed but I recall that some time in the past we didn't see this WebSocket requests which are in reality the connection part of a normal HTTP request but the request time was shown in the actual HTTP request entry in the list.
that's how it should be again.

@brianquinlan
Copy link

@bkonyi and @brianquinlan the network traffic in question comes from the VmService's getSocketProfile RPC. What are your thoughts WRT to how this data should actually be classified if it is not web socket traffic? Do you have a proposed solution to this issue?

From #8010 it looks like all network requests are being classified as WebSocket requests. I don't know if DevTools or VmService is doing the wrong thing here.

@kenzieschmoll kenzieschmoll added P1 high priority issues at the top of the work list, actively being worked on. and removed P2 important to work on, but not at the top of the work list. labels Jul 12, 2024
@kenzieschmoll
Copy link
Member

DevTools is just reading the data we get from the VM service. @derekxu16 or @bkonyi can you take a look at the vm_service side of this and see if anything has changed recently that could be causing this? I know there was some protocol-side work on network related code in the last ~6 months or so.

@julemand101
Copy link
Author

@kenzieschmoll

DevTools is just reading the data we get from the VM service

Technically true but I think DevTools are at fault here.

If we start from this method that gets called with list of SocketStatistic (which are of the type TCP or UDP based on the documentation https://pub.dev/documentation/vm_service/latest/vm_service/SocketStatistic/socketType.html . For me, it look very much like those SocketStatistic objects are statistic about raw sockets and not websocket)):

  /// Update or add all [requests] and [sockets] to the current requests.
  ///
  /// If the entry already exists then it will be modified in place, otherwise
  /// a new [HttpProfileRequest] will be added to the end of the requests lists.
  ///
  /// [notifyListeners] will only be called once all [requests] and [sockets]
  /// have be updated or added.
  void updateOrAddAll({
    required List<HttpProfileRequest> requests,
    required List<SocketStatistic> sockets,
    required int timelineMicrosOffset,
  }) {
    _updateOrAddRequests(requests);
    _updateWebSocketRequests(sockets, timelineMicrosOffset);
    notifyListeners();
  }

void updateOrAddAll({
required List<HttpProfileRequest> requests,
required List<SocketStatistic> sockets,
required int timelineMicrosOffset,
}) {
_updateOrAddRequests(requests);
_updateWebSocketRequests(sockets, timelineMicrosOffset);
notifyListeners();
}
/// Update or add the [request] to the [requests] depending on whether or not
/// its [request.id] already exists in the list.
///
void _updateOrAddRequests(List<HttpProfileRequest> requests) {
for (int i = 0; i < requests.length; i++) {
final request = requests[i];
_updateOrAddRequest(request);
}
}
void _updateOrAddRequest(HttpProfileRequest request) {
final wrapped = DartIOHttpRequestData(
request,
requestFullDataFromVmService: false,
);
if (!_requestsById.containsKey(request.id)) {
_requestsById[wrapped.id] = wrapped;
value.add(wrapped);
} else {
// If we override an entry that is not a DartIOHttpRequestData then that means
// the ids of the requestMapping entries may collide with other types
// of requests.
assert(_requestsById[request.id] is DartIOHttpRequestData);
(_requestsById[request.id] as DartIOHttpRequestData).merge(wrapped);
}
}

But for some reason, we just send those sockets to this _updateWebSocketRequests method:

  void _updateWebSocketRequests(
    List<SocketStatistic> sockets,
    int timelineMicrosOffset,
  ) {
    for (final socket in sockets) {
      final webSocket = WebSocket(socket, timelineMicrosOffset);

      if (_requestsById.containsKey(webSocket.id)) {
        final existingRequest = _requestsById[webSocket.id];
        if (existingRequest is WebSocket) {
          existingRequest.update(webSocket);
        } else {
          // If we override an entry that is not a Websocket then that means
          // the ids of the requestMapping entries may collide with other types
          // of requests.
          assert(existingRequest is WebSocket);
        }
      } else {
        value.add(webSocket);
        // The new [sockets] may contain web sockets with the same ids as ones we
        // already have, so we remove the current web sockets and replace them with
        // updated data.
        _requestsById[webSocket.id] = webSocket;
      }
    }
  }

void _updateWebSocketRequests(
List<SocketStatistic> sockets,
int timelineMicrosOffset,
) {
for (final socket in sockets) {
final webSocket = WebSocket(socket, timelineMicrosOffset);
if (_requestsById.containsKey(webSocket.id)) {
final existingRequest = _requestsById[webSocket.id];
if (existingRequest is WebSocket) {
existingRequest.update(webSocket);
} else {
// If we override an entry that is not a Websocket then that means
// the ids of the requestMapping entries may collide with other types
// of requests.
assert(existingRequest is WebSocket);
}
} else {
value.add(webSocket);
// The new [sockets] may contain web sockets with the same ids as ones we
// already have, so we remove the current web sockets and replace them with
// updated data.
_requestsById[webSocket.id] = webSocket;
}
}
}

Where we end up creating WebSocket objects (not the type from dart:io but its own type in DevTools). If we take a look at this WebSocket class we can see where the problems in the GUI comes from:

class WebSocket extends NetworkRequest {
  WebSocket(this._socket, this._timelineMicrosBase);

  // ...

  @override
  String get contentType => 'websocket';

  @override
  String get type => 'ws';

  String get socketType => _socket.socketType;

  @override
  String get uri => _socket.address;

  @override
  int get port => _socket.port;

  // ...

  // TODO(kenz): is this always GET? Chrome DevTools shows GET in the response
  // headers for web socket traffic.
  @override
  String get method => 'GET';

  // TODO(kenz): is this always 101? Chrome DevTools lists "101" for WS status
  // codes with a tooltip of "101 Web Socket Protocol Handshake"
  @override
  String get status => '101';

  // ...
}

class WebSocket extends NetworkRequest {
WebSocket(this._socket, this._timelineMicrosBase);
int _timelineMicrosBase;
SocketStatistic _socket;
int timelineMicrosecondsSinceEpoch(int micros) {
return _timelineMicrosBase + micros;
}
void update(WebSocket other) {
_socket = other._socket;
_timelineMicrosBase = other._timelineMicrosBase;
notifyListeners();
}
@override
String get id => _socket.id;
@override
Duration? get duration {
final endTime = _socket.endTime;
if (endTime == null) {
return null;
}
return Duration(microseconds: endTime - _socket.startTime);
}
@override
DateTime get startTimestamp => DateTime.fromMicrosecondsSinceEpoch(
timelineMicrosecondsSinceEpoch(_socket.startTime),
);
@override
DateTime? get endTimestamp {
final endTime = _socket.endTime;
return endTime != null
? DateTime.fromMicrosecondsSinceEpoch(
timelineMicrosecondsSinceEpoch(endTime),
)
: null;
}
DateTime? get lastReadTimestamp {
final lastReadTime = _socket.lastReadTime;
return lastReadTime != null
? DateTime.fromMicrosecondsSinceEpoch(
timelineMicrosecondsSinceEpoch(lastReadTime),
)
: null;
}
DateTime? get lastWriteTimestamp {
final lastWriteTime = _socket.lastWriteTime;
return lastWriteTime != null
? DateTime.fromMicrosecondsSinceEpoch(
timelineMicrosecondsSinceEpoch(lastWriteTime),
)
: null;
}
@override
String get contentType => 'websocket';
@override
String get type => 'ws';
String get socketType => _socket.socketType;
@override
String get uri => _socket.address;
@override
int get port => _socket.port;
// TODO(kenz): what determines a web socket request failure?
@override
bool get didFail => false;
int get readBytes => _socket.readBytes;
int get writeBytes => _socket.writeBytes;
// TODO(kenz): is this always GET? Chrome DevTools shows GET in the response
// headers for web socket traffic.
@override
String get method => 'GET';
// TODO(kenz): is this always 101? Chrome DevTools lists "101" for WS status
// codes with a tooltip of "101 Web Socket Protocol Handshake"
@override
String get status => '101';
@override
bool get inProgress => false;
@override
bool operator ==(Object other) => other is WebSocket && id == other.id;
@override
int get hashCode => id.hashCode;
}

So this type will always just call these events for ws or websocket. Even if they originally comes from SocketStatistic that have nothing to do with websockets.

@julemand101
Copy link
Author

My guess is that this mistake got implemented with #2191 where the code looks like it mixing up what is socket and what a websocket is. Maybe the confusion happen because the developers was more focused on web development where the word "socket" more easily gets translated to "websocket".

But I can't find any traces of SocketStatistic have ever meant websocket (tried look though vm_service). Yes, there can be a websocket inside a normal socket but to then conclude all sockets must be websockets... that is wrong. :)

@bkonyi
Copy link
Contributor

bkonyi commented Jul 12, 2024

@julemand101 yep, that's correct! SocketStatistic is reporting details about Socket objects, not WebSocket, so the above code is making a bad assumption.

@kenzieschmoll
Copy link
Member

@bkonyi do we have any information from the VM about web socket traffic? From the devtools side, we request two things from the VM:

  1. an HttpProfile
  2. a SocketProfile

Can one or both of these profiles contain web socket traffic? and if so, how is web socket traffic distinguished from other types of network traffic within those profile(s)?

@laterdayi
Copy link

Looking forward to solving it, the problem right now is very difficult for development debugging

bkonyi added a commit to bkonyi/devtools that referenced this issue Jul 16, 2024
@bkonyi bkonyi closed this as completed in f440b48 Jul 17, 2024
kenzieschmoll pushed a commit to kenzieschmoll/devtools that referenced this issue Jul 17, 2024
… connections (flutter#8061)

* Fix issue where socket profile data was being displayed as web socket connections

Fixes flutter#3033
Piinks pushed a commit that referenced this issue Aug 5, 2024
* Fix overlapping UI on disconnect (#8057)

* Fix issue where socket profile data was being displayed as web socket connections (#8061)

* Fix issue where socket profile data was being displayed as web socket connections

Fixes #3033

* Prepare cherry-pick release - DevTools 2.37.2

---------

Co-authored-by: Ben Konyi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fix it friday P1 high priority issues at the top of the work list, actively being worked on. screen: network Issues with the Network screen.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants