Skip to content

[CP] [ dart:io ] Fix minor bugs in network profiler | https://dart-review.googlesource.com/c/sdk/+/375800 #56273

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.

Sign up for GitHub

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

Jump to bottom
Closed
bkonyi opened this issue Jul 17, 2024 · 5 comments
Closed

[CP] [ dart:io ] Fix minor bugs in network profiler | https://dart-review.googlesource.com/c/sdk/+/375800 #56273

bkonyi opened this issue Jul 17, 2024 · 5 comments
Assignees
@vsmenon @itsjustkevin @mit-mit @athomas
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. cherry-pick-approved Label for approved cherrypick request cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. cherry-pick-review Issue that need cherry pick triage to approve triaged Issue has been triaged by sub team type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@bkonyi
Copy link
Contributor

bkonyi commented Jul 17, 2024

Commit(s) to merge

791163e

Target

beta

Prepared changelist for beta/stable

https://dart-review.googlesource.com/c/sdk/+/376125

Issue Description

  • Socket data in the DevTools network profiler displays addresses as InternetAddress(<address>, <address type>)
  • If HttpClientResponse.listen isn't invoked, HTTP requests never show up as completed in the network profiler

What is the fix

  • Call .address on InternetAddress instead of .toString()
  • Move HTTP response completion logic to complete when the HttpClientResponse is drained, not only

Why cherry-pick

The network page is difficult to use and confusing for Socket traffic without this change. We have several users blocked by this broken experience. The issue had 10 upvotes, several other duplicate issues filed, and chatter on Discord.

Risk

Low

Issue link(s)

flutter/devtools#8050, flutter/devtools#3033, flutter/devtools#6642

Extra Info

No response
@bkonyi bkonyi added the cherry-pick-review Issue that need cherry pick triage to approve label Jul 17, 2024
@bkonyi
Copy link
Contributor Author

bkonyi commented Jul 17, 2024

cc @kenzieschmoll

@dart-github-bot
Copy link
Collaborator

Summary: This issue addresses two bugs in the Dart network profiler: Socket data displays addresses in a confusing format, and HTTP requests don't show as completed if HttpClientResponse.listen isn't called. The fix improves the user experience by displaying addresses correctly and ensuring HTTP requests are properly marked as completed.

@dart-github-bot dart-github-bot added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Jul 17, 2024
@a-siva a-siva added the triaged Issue has been triaged by sub team label Jul 17, 2024
@kenzieschmoll kenzieschmoll mentioned this issue Jul 17, 2024
Closed
@a-siva
Copy link
Contributor

a-siva commented Jul 17, 2024

lgtm

@bkonyi
Copy link
Contributor Author

bkonyi commented Jul 19, 2024

@itsjustkevin is this safe to merge?

@itsjustkevin itsjustkevin added the cherry-pick-approved Label for approved cherrypick request label Jul 19, 2024
@itsjustkevin
Copy link
Contributor

Yes @bkonyi please merge.

copybara-service bot pushed a commit that referenced this issue Jul 19, 2024
@bkonyi
[beta] [ dart:io ] Fix minor bugs in network profiler
35a595f 
Fixes issues where:
 - Socket profile addresses were set using `InternetAddress.toString()`
   instead of `InternetAddress.address`
 - HTTP requests were not being marked as completed if `listen(...)` was
   not called on the `HttpClientResponse`
 - An exception could be thrown from profiling code if an HTTP response
   was completed after it was already completed with an error

Change-Id: I572928b77c4b1eb241acc7ec29f0e95dd7b2da8a
CoreLibraryReviewExempt: Only impacts dart:io network profiling logic
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/375800
Cherry-pick-request: #56273
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/376125
Commit-Queue: Ben Konyi <[email protected]>
Reviewed-by: Kenzie Davisson <[email protected]>
@lrhn lrhn removed the triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. label Jul 19, 2024
@itsjustkevin itsjustkevin added the cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. label Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vsmenon vsmenon

@itsjustkevin itsjustkevin

@mit-mit mit-mit

@athomas athomas

Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. cherry-pick-approved Label for approved cherrypick request cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. cherry-pick-review Issue that need cherry pick triage to approve triaged Issue has been triaged by sub team type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@vsmenon @lrhn @a-siva @itsjustkevin @mit-mit @athomas @bkonyi @dart-github-bot