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

Fix connection timeouts if server doesn't send something in time #525

Merged
merged 2 commits into from
May 9, 2020

Conversation

jaime-ez
Copy link
Collaborator

@jaime-ez jaime-ez commented May 1, 2020

This prevents the client from hanging when a user tries to write to a record with ack but he only has read access.

…n trying to set with ack the error callback was not triggered and the client hanged indefinitely.
@jaime-ez
Copy link
Collaborator Author

jaime-ez commented May 1, 2020

There is still an issue with the emitter on L.562, is not being called properly when the record.set call is without callback (given the read and not write permission)

@yasserf
Copy link
Contributor

yasserf commented May 1, 2020

Thank you, I’m currently offline (off tech offline) for a week so will get back to this then!

@yasserf
Copy link
Contributor

yasserf commented May 5, 2020

@jaime-ez if you could write a basic client example that shows the issue I can fix it up. Unless it isn’t something easily reproducible, then let me know steps and I’ll put in a test to reproduce it that way (next week)

@jaime-ez
Copy link
Collaborator Author

jaime-ez commented May 5, 2020

No problem, it's simple to reproduce:

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)
})

@yasserf
Copy link
Contributor

yasserf commented May 6, 2020

Amazing thank you, will look into this in a few days!i

:100644 100644 29ae60b d2b44d6 M	src/connection/socket-factory.ts
@yasserf
Copy link
Contributor

yasserf commented May 8, 2020

lgtm, would merge in except its blocked on

There is still an issue with the emitter on L.562, is not being called properly when the record.set call is without callback (given the read and not write permission)

we could break that out into a separate issue and I can this merged and released, not sure if I'm going to be able to look into it right now due

@jaime-ez
Copy link
Collaborator Author

jaime-ez commented May 8, 2020

Ok for breaking it into a separate issue. Let me know if you need any help with it, last time I wasnt able to solve it.

@yasserf
Copy link
Contributor

yasserf commented May 8, 2020

Will do thanks!

@yasserf yasserf changed the title fix record set with ack permission error Fix connection timeouts if server doesn't send something in time May 9, 2020
@yasserf yasserf merged commit 20572e2 into deepstreamIO:master May 9, 2020
@yasserf
Copy link
Contributor

yasserf commented May 9, 2020

Incredible thank you

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