graphile / graphile/graphile-engine

Race condition when closing LDS

Open
#755 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
764
Forks
128
PR merge metrics
No merged PRs in 30d

Description

### Summary

In our integration test suite, we do teardown the LDS Live Source by calling `schema.__pgLdsSource?.close()`. This does however not work consistently, and from time to time (apparently when the server - that also runs the db in a docker container - is under higher load) it fails to stop the LDS loop and the test job (github action) is only stopped by timing out, failing the build step.

In the meantime, it does keep logging
> Error during LDS loop: Pool has been closed

I believe I tracked this down to a bug in https://github.com/graphile/graphile-engine/blob/6692c2a65f406c55c3d1edfff51c8f7cc2296956/packages/lds/src/index.ts#L96-L101
```ts
async function loop() {
try {
const rows = await client.getChanges(null, 500);

} catch (e) {
console.error("Error during LDS loop:", e.message);
// Recovery time...
loopTimeout = setTimeout(loop, sleepDuration * 10);
return;
}
loopTimeout = setTimeout(loop, sleepDuration);
}
loop();
```

While the `client.getChanges` query is `await`ed, no timeout is scheduled and the `clearTimeout` will be ineffective, so the loop will start over even after `.close()` has been called.

### Steps to reproduce

By setting the `sleepDuration` to a minimum value, the loop spends much more time in the query so a `close()` will hit the sleep much less likely.

### Possible Solution

I can see a few approaches:
* just set a `closed` flag and check this before calling `setTimeout` again
* `.unref()` each timer? I think this might have worked in my case, but probably not a generic solution
* check whether the `client.pool` is closed before calling `getChanges()`, and breaking out of the loop
* check whether the `client.pool` is closed in the `catch` handler of the loop, breaking out from there
* a specific "client closed" exception that is handled differently in the `catch` of the loop, breaking out from there

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.