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

Update session-builder.ts #78

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zEvAngelz
Copy link

fix
create_session error SignalProtocolAddress { _name: 'xxx', _deviceId: 1 } TypeError: Cannot read properties of null (reading 'version') at Function.deserialize (/root/xxx/node_modules/@privacyresearch/libsignal-protocol-typescript/lib/session-record.js:56:18) at SessionBuilder.<anonymous> (/root/xxx/node_modules/@privacyresearch/libsignal-protocol-typescript/lib/session-builder.js:64:57) at Generator.next (<anonymous>) at fulfilled (/root/xxx/node_modules/@privacyresearch/libsignal-protocol-typescript/lib/session-builder.js:24:58) at processTicksAndRejections (node:internal/process/task_queues:96:5)

fix

create_session error SignalProtocolAddress { _name: 'xxx', _deviceId: 1 } TypeError: Cannot read properties of null (reading 'version')
    at Function.deserialize (/root/xxx/node_modules/@privacyresearch/libsignal-protocol-typescript/lib/session-record.js:56:18)
    at SessionBuilder.<anonymous> (/root/xxx/node_modules/@privacyresearch/libsignal-protocol-typescript/lib/session-builder.js:64:57)
    at Generator.next (<anonymous>)
    at fulfilled (/root/xxx/node_modules/@privacyresearch/libsignal-protocol-typescript/lib/session-builder.js:24:58)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
Copy link
Contributor

@rolfeschmidt rolfeschmidt left a comment

Choose a reason for hiding this comment

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

This looks reasonable modulo my small comment, but I want to understand the need for this change better.

The StorageType interface declares loadSession to return Promise<string | undefined>. Presumably your storage service is returning a string that is actually null. Is there a semantic difference between null and undefined return values in your storage implementation? (e.g. null means "I know about this session but it is gone" and undefined means "this session never existed") If so, where do you use this distinction?

If not, is it possible to ensure that your storage always returns undefined instead of null?

If we make this change and move the responsibility to distinguish null/undefined to the SDK then I think we need a bigger review of the code. (But could merge this as a start.)

@@ -55,7 +55,7 @@ export class SessionBuilder {
const address = this.remoteAddress.toString()
const serialized = await this.storage.loadSession(address)
let record: SessionRecord
if (serialized !== undefined) {
if (serialized !== undefined && serialized != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer checking serialized !== null to avoid the implicit conversions. (Since null == undefined is true we could just say serialized != undefined here and it would eliminate null, but the !== makes things more clear IMO.)

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

Successfully merging this pull request may close these issues.

None yet

2 participants