Client hangs on write when no permissions available

[yasserf]

On server set these permissions:

presence:
  "*":
    allow: true
record:
  "*":
    create: true
    write: user.id === 'ok'
    read: true
    delete: true
    listen: true
    notify: true
event:
  "*":
    publish: true
    subscribe: true
    listen: true
rpc:
  "*":
    provide: true
    request: true

Then execute this on a file, expected behaviour described on code:

'use strict'

const { DeepstreamClient } = require('@deepstream/client')

const readWriteClient = new DeepstreamClient('localhost:6020/deepstream')
const readClient = new DeepstreamClient('localhost:6020/deepstream')

// this client can read and write
readWriteClient.login({ username: 'ok' })

// this client can only read
readClient.login()

readWriteClient.on('connectionStateChanged', (s) => {
  if (s === 'OPEN') {
    // we use the client that has write and read permission
    readWriteClient.record.getRecord('test').whenReady((rec) => {
      // everything works as intended
      console.log('record data', rec.get())

      rec.set('hello', 'world', (e) => {
        console.log('no error on set', e === null)
        console.log('updated record', rec.get())

        // here is the issue:
        // now with the client that does not have write permission, only read
        readClient.record.getRecord('test').whenReady((r) => {
          // we can read as intended
          console.log('can read', r.get())

          // we can not write, but an error should be called or emitted, instead the client just hangs
          r.set('bye', 'world', (err) => {
            // THIS SHOULD BE CALLED
            console.log('error is never called', err)
          })
        })
      })
    })
  }
})

readClient.on('error', (e) => {
  // OR AN ERROR EMITED HERE
  console.log('no error emitted', e)
})

Originally posted by @jaime-ez in #525 (comment)

[jaime-ez]

Yasserf, an update on this - the title should say hang :) -

The case with write callback has been solved in the previous pull request. However when there is no callback, I thought the client should emit the error, but it is the record that emits the error!

Updated file to be executed (same permissions as before) - comments in file explain the situation:

'use strict'

const { DeepstreamClient } = require('@deepstream/client')

const readWriteClient = new DeepstreamClient('localhost:6020/deepstream')
const readClient = new DeepstreamClient('localhost:6020/deepstream')

// this client can read and write
readWriteClient.login({ username: 'ok' })

// this client can only read
readClient.login()

readWriteClient.on('connectionStateChanged', (s) => {
  if (s === 'OPEN') {
    // we use the client that has write and read permission
    readWriteClient.record.getRecord('test').whenReady((rec) => {
      // everything works as intended
      console.log('record data', rec.get())

      rec.set('hello', 'world', (e) => {
        console.log('no error on set', e === null)
        console.log('updated record', rec.get())
        rec.discard()

        // HERE IS THE ISSUE:
        // now with the client that does not have write permission, only read
        readClient.record.getRecord('test').whenReady((r) => {
          // we can read as intended
          console.log('can read', r.get())

          // we can not write, but an error should be emitted, instead the client just hangs
          r.set('bye', 'world')
          // BUT THE RECORD EMITS THE ERROR!
          r.on('error', (e) => {
            console.log('HERE WE EMIT', e)
          })
        })
      })
    })
  }
})

readClient.on('error', (e) => {
  // NOTHING HAPPENS HERE
  console.log('no error emitted', e)
})

In summary, the record emits the erorr, and thats ok, but I think the error should also be propagated to the client in order to centralize error subscriptions.

Looking at the code, an option could be:

1. Pass the client emitter as argument (which one? third?) to the record-handler constructor here

2. Pass the client emitter to the record constructor here

3. and then implement the error propagation here

Do you agree the error should be propagated to the client?
Any comments/suggestions on how to implement it?

[yasserf]

Yasserf, an update on this - the title should say hang :) -

Thanks! 😅

Do you agree the error should be propagated to the client?

Probably yeah, since its a development error and should bubble.

The record (and almost the entire client) does has access to the logger which in turn emits errors.

public error (message: { topic: TOPIC } | Message, event?: EVENT | ALL_ACTIONS, meta?: string | JSONObject | Error): void {
    if (isEvent(event)) {
      if (event === EVENT.IS_CLOSED || event === EVENT.CONNECTION_ERROR) {
        this.emitter.emit('error', meta, EVENT[event], TOPIC[TOPIC.CONNECTION])
      }
    } else {
      const action = event ? event : (message as Message).action
      this.emitter.emit(
        'error',
        meta || message,
        (ACTIONS as any)[message.topic][action],
        TOPIC[message.topic]
      )
    }
  }

For example using it in record handler would be:

      this.services.logger.error(
        { topic: TOPIC.RECORD }, EVENT.RECORD_READ_ONLY_MODE, 'Attempting to set data when in readonly mode, ignoring'
      )

That can go directly in record core, since we don't care about context.

Does that make sense?

[yasserf]

Also relieved this isn't an issue! I just spent a couple hours fixing another record merge conflict bug (#519) and my brain was a bit 😱😅

[jaime-ez]

Excelent, I'll try to pass the record errors through the logger service then. Thanks!

…

On Sat, May 9, 2020, 21:53 Yasser Fadl ***@***.***> wrote: Also relieved this isn't an issue! I just spent a couple hours fixing another merge conflict and my brain was a bit 😱😅 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#527 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABF6ROBQVIB5XLBVFEORGJ3RQYCKLANCNFSM4M476ITA> .

[yasserf]

👍