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

Create a simple WebSocket interface #1128

Merged
merged 12 commits into from
Feb 20, 2024

Conversation

brianquinlan
Copy link
Collaborator

@brianquinlan brianquinlan commented Feb 7, 2024

  • This just defines the interface that will be implemented using dart:io, package:web and package:cupertino_http.

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@github-actions github-actions bot added the type-infra A repository infrastructure change or enhancement label Feb 7, 2024
@brianquinlan brianquinlan changed the title Create a SimpleWebSocket interface Create a simple WebSocket interface Feb 8, 2024
/// (e.g. 1006).
///
/// Errors will never appear in this [Stream].
Stream<WebSocketEvent> get events;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Stream and its relationship with the sendText/sendBytes/close methods is my biggest concern with this API.

As documented now, sendText, etc. throw if the WebSocket is closed. But the ClosedReceived event is delivered asynchronously so you can't rely on it being delivered in time to prevent those calls. For example:

timer = Timer.periodic(const Duration(seconds: 2), (timer) {
  websocket.sendText('<stock>GOOG</stock>');
});

await for (final event in websocket.events) {
  switch (event) {
  // Should probably do something with the stock quote.
  case CloseReceived:
    // This is not sufficient - the implementation may add `CloseReceived` to the `StreamController` and then
    // close the `StreamController` before this event is received. This probably means that, in practice, every
    // call to `sendText`/`sendBytes` will need to be guarded in a `try`/`catch`
    timer.cancel();
  }
}

Some alternatives:

  1. just throw away sendText/sendBytes after the WebSocket is closed - this is what JavaScript does
  2. use SyncStreamController (will that work?)
  3. make a synchronous callback for events instead of using a Stream.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think even if we synchronously surface a ClosedReceived there is still the possibility of a sendText failing to write because of a closed socket, though it may be significantly less less likely.

I would probably start with throwing, and we can consider an option to silently discard if that has a clear use case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think even if we synchronously surface a ClosedReceived there is still the possibility of a sendText failing to write because of a closed socket, though it may be significantly less less likely.

My plan was for sendText/sendBytes to not throw in that case. Any other behavior would be difficult on the web, which only allows send to throw if the connection isn't established.

I have a conformance test to verify that behavior (the tests are still very rough at this point).

I've updated the docs to make a note of that.

I would probably start with throwing, and we can consider an option to silently discard if that has a clear use case.

SGTM!

pkgs/web_socket/CHANGELOG.md Outdated Show resolved Hide resolved
/// (e.g. 1006).
///
/// Errors will never appear in this [Stream].
Stream<WebSocketEvent> get events;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think even if we synchronously surface a ClosedReceived there is still the possibility of a sendText failing to write because of a closed socket, though it may be significantly less less likely.

I would probably start with throwing, and we can consider an option to silently discard if that has a clear use case.

pkgs/web_socket/pubspec.yaml Outdated Show resolved Hide resolved
@brianquinlan brianquinlan merged commit 75e01f4 into dart-lang:master Feb 20, 2024
25 checks passed
@brianquinlan brianquinlan deleted the websocket-api branch February 20, 2024 20:56
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Feb 21, 2024
…, sse, stream_channel, tools, vector_math, web, webdev, yaml_edit

Revisions updated by `dart tools/rev_sdk_deps.dart`.

async (https://github.com/dart-lang/async/compare/24266ca..6cdbc41):
  6cdbc41  2024-02-15  Kevin Moore  Update to latest lints, require Dart 3.2 (dart-archive/async#267)

browser_launcher (https://github.com/dart-lang/browser_launcher/compare/74a0efe..7956230):
  7956230  2024-02-16  sigmundch  Add extra flags to disable throttling behavior. (dart-archive/browser_launcher#55)

dartdoc (https://github.com/dart-lang/dartdoc/compare/7e171fc..7a9df65):
  7a9df65f  2024-02-20  Parker Lougheed  Add fallback text for sidebar failing to load (dart-lang/dartdoc#3643)
  9bcabb50  2024-02-20  Parker Lougheed  Fix missing left sidebar on extension type pages (dart-lang/dartdoc#3662)
  e8b8faa2  2024-02-16  Sam Rawlins  Include extension types in 'implementers' list (dart-lang/dartdoc#3658)

http (https://github.com/dart-lang/http/compare/6d9f9ef..ce0de37):
  ce0de37  2024-02-21  Derek Xu  Populate package:http_profile (dart-lang/http#1046)
  75e01f4  2024-02-20  Brian Quinlan  Create a simple WebSocket interface (dart-lang/http#1128)

markdown (https://github.com/dart-lang/markdown/compare/c2b8429..d735b0b):
  d735b0b  2024-02-21  Tom Yeh  Fix `#578`: list with checkbox mixed with empty lines (dart-lang/markdown#583)
  6efe141  2024-02-14  Kevin Moore  Migrate example to pkg:web, update minimum required Dart version (dart-lang/markdown#582)

protobuf (https://github.com/dart-lang/protobuf/compare/a293fb9..f085bfd):
  f085bfd  2024-02-20  Ömer Sinan Ağacan  Fix message_set.dart copyright year (google/protobuf.dart#912)

sse (https://github.com/dart-lang/sse/compare/af7d8d0..13ec752):
  13ec752  2024-02-20  Kevin Moore  blast_repo fixes (dart-lang/sse#104)
  2830dc9  2024-02-16  Kevin Moore  Support the latest pkg:web, require Dart 3.3 (dart-lang/sse#103)

stream_channel (https://github.com/dart-lang/stream_channel/compare/851336f..e02a5dd):
  e02a5dd  2024-02-16  Kevin Moore  Require Dart 3.3, update and fix lints (dart-lang/stream_channel#100)
  e62706e  2024-02-16  Kevin Moore  blast_repo fixes (dart-lang/stream_channel#101)

tools (https://github.com/dart-lang/tools/compare/2ef7673..9f4e6a4):
  9f4e6a4  2024-02-16  Elias Yishak  Helper to resolve dart version for clients of analytics (dart-lang/tools#233)
  8323b21  2024-02-13  Elias Yishak  New event added for sending analytics within package on errors (dart-lang/tools#229)

vector_math (https://github.com/google/vector_math.dart/compare/cb976c7..3706feb):
  3706feb  2024-02-18  dependabot[bot]  Bump dart-lang/setup-dart from 1.6.0 to 1.6.2 (google/vector_math.dart#313)

web (https://github.com/dart-lang/web/compare/a54a1f0..975e55c):
  975e55c  2024-02-15  Kevin Moore  Add TrustedTypes (dart-lang/web#173)
  0447807  2024-02-15  Srujan Gaddam  Add info on generation conventions (dart-lang/web#171)

webdev (https://github.com/dart-lang/webdev/compare/629c632..51b5484):
  51b54843  2024-02-14  Elliott Brooks  Implement `setFlag` for 'pause_isolates_on_start' (dart-lang/webdev#2373)

yaml_edit (https://github.com/dart-lang/yaml_edit/compare/2a9a11b..82ab64d):
  82ab64d  2024-02-21  Danny Tuppeny  Fix line endings for inserted maps on Windows (dart-lang/yaml_edit#66)
  6906ac4  2024-02-20  Devon Carew  update the publish workflow (dart-lang/yaml_edit#67)

Change-Id: I246c393586e3d6239925ac3cf3a6a245d86a2bf5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/353581
Reviewed-by: Kevin Moore <kevmoo@google.com>
Commit-Queue: Devon Carew <devoncarew@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-infra A repository infrastructure change or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants