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

Make ICipher.decrypt async #1311

Merged
merged 13 commits into from
Jun 5, 2023

Conversation

lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian commented May 30, 2023

Note: This is based on top of #1246; please review that one first.

This makes ICipher.decrypt async, in preparation for using the (async-only) SubtleCrypto.decrypt method in #1292.

The changes of note are:

  • making RealtimeChannel#processMessage async — involves adding a queue to synchronise access to this method
  • making the public {Message, PresenceMessage}#{fromEncoded, fromEncodedArray} methods async

See commit messages for more details.

Resolves #1293.

@github-actions github-actions bot temporarily deployed to staging/pull/1311/features May 30, 2023 12:05 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1311/bundle-report May 30, 2023 12:06 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1311/typedoc May 30, 2023 12:06 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1311/features May 31, 2023 19:42 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1311/bundle-report May 31, 2023 19:43 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1311/typedoc May 31, 2023 19:43 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1311/features May 31, 2023 20:01 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1311/typedoc May 31, 2023 20:02 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1311/bundle-report May 31, 2023 20:03 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1311/features June 1, 2023 13:58 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1311/bundle-report June 1, 2023 13:59 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1311/typedoc June 1, 2023 13:59 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1311/features June 1, 2023 14:37 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1311/bundle-report June 1, 2023 14:38 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1311/typedoc June 1, 2023 14:39 Inactive
@lawrence-forooghian lawrence-forooghian marked this pull request as ready for review June 1, 2023 17:53
Copy link
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

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

LGTM, nicely done :)

Base automatically changed from convert-crypto-to-TypeScript to integration/v2 June 5, 2023 20:09
As part of #1293 (making ICipher.decrypt asynchronous), we will be
making this method async, and another method will wish to wait on its
completion. As such, this should no longer be named as if it were only
an informative callback.
This is preparation for #1293 (making ICipher.decrypt asynchronous).
This will require us to make RealtimeChannel#processMessage asynchronous.

Since RealtimeChannel#processMessage reads from and writes to the
_decodingContext.baseEncodedPreviousPayload property of the channel, we
need to ensure that, once this method becomes asynchronous, we serialise
access to this method — that is, we wait for one call to complete before
performing the next.

To do this, we need to introduce a queue. I decided to put this queue at
the level of the ConnectionManager instead of RealtimeChannel. This is
because ConnectionManager also has its own logic for deciding whether a
message should be processed — specifically, whether it comes from the
current transport — and I thought it made sense to evaluate these
conditions at the moment we pass the message to the channel. I’m not
100% sure this is the right choice, though, since it means that the
synchronisation is now the concern of three components
(ConnectionManager, Channels, RealtimeChannel) when it instead could be
the concern of just RealtimeChannel. But we can always revisit this.

The handling of the case where ConnectionManager#processChannelMessage
throws an error is copied from the places where this error was
previously handled — namely, WebSocketTransport.onWsData and
CometTransport.onData, both of which log an error message without
affecting the processing of subsequent messages.

(Note also that this marks the first use of `async` or promises
internally in the library. We have avoided this until now because we
were not guaranteed to be running in browsers with Promise support, but
since the library will _only_ be exposing a Promise API as of #1199,
which is also scheduled for version 2.0, we’re fine to start doing so
now.)
Preparation for #1293 (making ICipher.decrypt asynchronous).
These are the tests that call RealtimeChannel#processMessage at their
top level. This is preparation for making that method asynchronous as
part of #1293 (making ICipher.decrypt asynchronous).
Preparation for #1293 (making ICipher.decrypt asynchronous).
Preparation for #1293 (making ICipher.decrypt asynchronous). This is an
unavoidable public API change.

Note that I haven’t bothered exposing a callbacks version of these
methods, since as of #1199 — which is also scheduled for version 2.0 —
the library will only be exposing a Promise API.
Preparation for #1293 (making ICipher.decrypt asynchronous).
Preparation for #1293 (making ICipher.decrypt asynchronous).
Preparation for #1293 (making ICipher.decrypt asynchronous).
Preparation for #1293 (making ICipher.decrypt asynchronous).
In preparation for using the (async-only) SubtleCrypto.decrypt method
in #1292.

Resolves #1293.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants