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

Does the leader need to go through socket? #28

Closed
ronag opened this issue Mar 7, 2021 · 4 comments
Closed

Does the leader need to go through socket? #28

ronag opened this issue Mar 7, 2021 · 4 comments

Comments

@ronag
Copy link
Contributor

ronag commented Mar 7, 2021

Couldn't the leader use the db directly?

@ronag
Copy link
Contributor Author

ronag commented Mar 7, 2021

I guess I don't understand this part:

party/index.js

Lines 104 to 114 in 6d5dc4f

if (client.isFlushed()) {
return
}
const sock = net.connect(sockPath)
pump(sock, client.createRpcStream(), sock, function () {
// TODO: err?
})
client.once('flush', function () {
sock.destroy()
})

@ronag
Copy link
Contributor Author

ronag commented Mar 7, 2021

Also, are we missing a ref: socket here?

party/index.js

Line 109 in 6d5dc4f

pump(sock, client.createRpcStream(), sock, function () {

Actually why do we even need ref: socket? pipeline should/will destroy.

@vweevers
Copy link
Member

vweevers commented Mar 11, 2022

Couldn't the leader use the db directly?

It does, 99% of the time.

I guess I don't understand this part

That's to counter the fact that the party starts after the publicly consumed db (i.e. multileveldown.client()) has opened itself. So at that point there could already be pending requests (from e.g. a db.put() call) that, even when the db is a leader, must be flushed, as if the db is not a leader. It's ugly and I want to refactor this - once I understand the ref thing.

As for why we don't need ref in this code path: I think because we're expecting to have to flush only a few early requests, after which the socket can be destroyed.

@vweevers
Copy link
Member

Actually why do we even need ref: socket? pipeline should/will destroy.

If a ref is provided, we unref that after having received responses to all pending requests, meaning there are no active db operations left that should keep the event loop alive (matching regular level behavior). Once a new db operation is made, we re-ref.

I discovered a small bug here: we don't initially unref. You must make at least one db operation for it to work. So this hangs:

const name = Date.now().toString()

const db1 = party(name)
const db2 = party(name)

await db1.open()
await db2.open()

console.log('opened')

// Comment this out to not make the event loop hang
// await db1.put('a', 'b')
// console.log(await db2.get('a'))

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

No branches or pull requests

2 participants