-
Notifications
You must be signed in to change notification settings - Fork 55
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
asynchronous
#1293
Comments
➤ Automation for Jira commented: The link to the corresponding Jira issue is https://ably.atlassian.net/browse/SDK-3604 |
@owenpearson would you mind laying out your thoughts re continuing to provide a synchronous version of the |
Spoke to @owenpearson today and he thinks we'll be fine to only offer an asynchronous version of |
In preparation for using SubtleCrypto.decrypt, which returns a promise. Resolves #1293.
In preparation for using SubtleCrypto.decrypt, which returns a promise. Resolves #1293.
In preparation for using SubtleCrypto.decrypt, which returns a promise. Resolves #1293.
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.
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.
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 (async-only) SubtleCrypto.decrypt. Resolves #1293.
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).
Preparation for #1293 (making ICipher.decrypt asynchronous).
Preparation for #1293 (making ICipher.decrypt asynchronous).
Preparation for #1293 (making ICipher.decrypt asynchronous).
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).
Preparation for #1293 (making ICipher.decrypt asynchronous).
Preparation for #1293 (making ICipher.decrypt asynchronous).
Preparation for #1293 (making ICipher.decrypt asynchronous).
Closed in #1311. |
The
SubtleCrypto.decrypt
method that we wish to start using in #1292 is an asynchronous-only API — it returns aPromise
.Our current internal cipher API (as described by the
ICipher
interface added in #1246) exposes a synchronousdecrypt
method. In light of the above, we will need to change this to an asynchronous method.This will necessitate internal changes to the library but will also affect its public API, since the
{Message, PresenceMessage}.{fromEncoded, fromEncodedArray}
static methods, which are currently synchronous, may need to decrypt data and hence in light of the above may now need to perform an asynchronous operation.We need to decide exactly how we'd like to change the public API — we will definitely need to have an asynchronous version of the aforementioned methods, but @owenpearson suggested that we might wish to also maintain synchronous variants (which, presumably, would fail in the case that the method needs to perform a decryption).
I've created #1302, in order to investigate the effort involved in the internal changes this change would require, so that we can then put an estimate on this issue.
The text was updated successfully, but these errors were encountered: