connection router to accomodate multithreading

[CoryGH]

connection router hook added to accommodate multithreaded servers

e.g. passing options.connectionRouter(connection) will allow for:

function connectionRouter(connection) {
    sendToThread(findRoute(connection), connection);
}

function sendToThread(threadId, connection) {
    ...wrap in packet for IPC and send to thread...
}

function threadMessage(message) {
    notListeningServerOnChildThread.newConnection(message.connection);
}

in order to preserve extant functionality while adding multi-threading support.

[UziTech]

Can you add a test and update docs?

[CoryGH]

Can you add a test and update docs?

Sure, I think I have a handle on your test system.

[UziTech]

it seems like multi threading with the connectionRouter option would only make use of ldapjs to start the server.

It seems like it might be easier to start the server and send connection to worker threads without ldapjs.

const net = require("net");
net.createServer(connectionRouter)

What are the benefits of using ldapjs to start the server when you are using notListeningServerOnChildThread to do all of the actual ldap work?

[CoryGH]

What are the benefits of using ldapjs to start the server when you are using notListeningServerOnChildThread to do all of the actual ldap work?

My aim was to make things minimally-invasive from an API standpoint. The connectionRouter function is called at the master thread level to tell the thread-manager (whichever it may be) to forward to a child thread which must then call newConnection(connection) in order to initialize the ldapjs side for the connection. It's designed around a forking design pattern but is extensible to cross-machine threading engines as well.

The server.listen function looks lightweight enough to do what you're suggesting (if what you mean is to create a net/tls server outside of ldapjs then use server.newConnection(connection) to leverage the ldapjs functionality with the socket (the change to make server.newConnection(connection) publicly-visible would still be needed, the connectionRouter piece wouldn't be needed at that point. Generally I shy away from moving entire members outside of libraries wholesale like that (in this case reinforced when I was reviewing server.newConnection(connection) because there's a lot of library-specific code in there leveraging the this/self reference to set the appropriate state and there's no real guarantee some of that logic or things similar to it from the patterns in the code won't get moved to the listen function in the future (that seems like a bigger commitment to me on the part of library developers/maintainers than adding a hook, though I could be wrong.)

[CoryGH]

This is a mess in trying to get around the tap test framework's thread handling, here's what I have thus far though - might need to take a break from it today but the code paths are being hit (failing test with "test unfinished" and I haven't pinned down why as of yet (I placed this in server.test.js, had to wrap the other tests in if (cluster.isMaster) { ... } as well to avoid them running on child threads.

tap.test('multithreading', function (t) {
  let threads = []
  const server = ldap.createServer({
    connectionRouter: (connection) => {
      if (cluster.isMaster) {
        threads[0].send({
          type: 'connection'
        }, connection)
      }
    }
  })
  if (cluster.isMaster) {
    let clientHolder = { client: null }
    for (let i = 0; i < 1; i++) {
      let thread = cluster.fork({
        id: i
      })
      thread.on('message', function (msg, connection) {
        switch (msg.type) {
          case 'connected':
            clientHolder.client.unbind()
            threads[0].disconnect()
            t.ok(true, 'client connected')
            t.end()
            server.close(() => t.end())
            break
        }
      })
      threads.push(thread)
    }
    t.ok(server)
    server.listen(5555, 'localhost', function () {
      t.ok(true, 'server listening on ' + server.url)

      server.getConnections(function (err, count) {
        t.error(err)
        t.equal(count, 0)
        setTimeout(() => {
          clientHolder.client = ldap.createClient({ port: 5555, url: server.url })
          clientHolder.client.on('connect', function () { })
        }, 1000)
      })
    })
  } else {
    process.on('message', (msg, connection) => {
      switch (msg.type) {
        case 'connection':
          server.newConnection(connection)
          process.send({
            type: 'connected'
          })
          break
      }
    })
  }
})

[CoryGH]

I would agree that if the server.listen() call can be guaranteed not to include state-specific bindings as the library evolves it would be better to just make server.newConnection(connection) publicly-visible without the connectionRouter(connection) hook.

[UziTech]

I think it would be easiest to mock the threads instead of trying to run the tests in actual threads. Just testing that connectionRouter option is called when it is supposed to be and that newConnection is public should work.

[CoryGH]

I think it would be easiest to mock the threads instead of trying to run the tests in actual threads. Just testing that connectionRouter option is called when it is supposed to be and that newConnection is public should work.

Would you like it with or without connectionRouter given the ability to bypass the listen function?

[UziTech]

Would you like it with or without connectionRouter given the ability to bypass the listen function?

Maybe two tests, one each way?

[CoryGH]

Would you like it with or without connectionRouter given the ability to bypass the listen function?

Maybe two tests, one each way?

Tests added and connectionRouter hook added back in.

[CoryGH]

@UziTech updated to fix linting issue

[jsumners]

Are you able to add some documentation? An example in the docs would be quite helpful.

[CoryGH]

Added examples in the examples directory for both multithreading via the connectionRouter hook in conjunction with newConnection(connection) and another by replacing the whole listen server to use newConnection(connection) for the node-ldapjs bindings. Only the net one was added to examples.md for the sake of brevity.

[jsumners]

Looks good to me once CI is green.

[UziTech]

@CoryGH you can run npm run lint to fix the linting issues.

[jsumners]

⚠️ This issue has been locked due to age. If you have encountered a recent
problem that seems to be covered by this issue, please open a new issue.

Please include a minimal reproducible example
when opening a new issue.