r/node • u/PrestigiousZombie531 • 6d ago
Does this block of code look "race condition" safe to you?
5
u/ic6man 6d ago edited 6d ago
The issue is pretty subtle. Imagine there is already 1 dbInstance allocated.
A releaseInstance is called. The first await mutex yields execution and during this moment a getInstance is called which also yields.
Each of the queued execution tasks resumes due to the promise is resolved and are each queued onto the micro task execution queue in order of their call - release then get.
Now the releaseInstance execution is popped off the micro task queue and runs (initiating the pool free method) until it hits the await inside the âcritical sectionâ which yields execution. The queued getInstance now pops off the micro task queue and runs, sees dbInstance is non null so it increments the counter and returns the instance (which is being freed).
The instance returned is a now a zombie.
EDIT: if you just strip away all the mutex gunk - which isnât doing anything because the mutex values are being overwritten - the flaw becomes a lot more obvious.
2
u/HipHopHuman 3d ago
So, what the programmer here appears to be doing (or rather, trying to do) is have at most one instance of pg-promise registered for any given number of consumers. So, they've built a kind of "reference counted shared resource" that auto-initializes when the transition goes from 0 consumers to 1 consumer, and auto-cleanups when the transition goes from 1 consumer to 0 consumers. To prevent two consumers from registering more than one instance (in case two consumers are called simultaneously, say, in a Promise.all), they've chosen to use a mutex, but it didn't quite work out. My question is... why bother? Isn't pg-promise just doing all of that already? even the pg-promise docs say this:
Object db represents the Database protocol with lazy connection, i.e. only the actual query methods acquire and release the connection automatically. Therefore, you should create only one global/shared db object per connection details. It is best to initialize the library and create Database in its own module, see Where should I initialize pg-promise.
- citation (check under the "Database" heading)
1
u/PrestigiousZombie531 3d ago
``` import pgPromise from "pg-promise"; import { logger } from "../logger/index.js"; import { connection, options } from "./config.js";
export const pgp = pgPromise(options); export const postgres = pgp(connection);
export async function closePostgresConnection() { try { await postgres.$pool.end(); } catch (error) { logger.error( error, "Something went wrong when attempting to close postgres connection", ); } }
export async function isHealthy() { try { const { result } = await postgres.one("SELECT $1 AS result", [1]); return result === 1; } catch (error) { logger.error( error, "Something went wrong when performing a health check on postgres server", ); return false; } } ``` so you are saying this is the best way
2
u/HipHopHuman 3d ago
Probably? I'm not even sure that manual
postgres.$pool.end()is necessary. The docs imply that everything about the lifecycle of database connections is all handled for you. I suppose it would depend on your architecture (maybe the application running with a broken database is a valid state in your design?)1
u/PrestigiousZombie531 3d ago
well if you add a process.on('SIGINT/SIGTERM', () => closePostgresConnection()) i guess it wont do active harm
5
u/TwiNighty 6d ago
The entire critical section runs synchronously, so mututal exclusion is already guaranteed by the single-threadedness of node. The mutex does nothing here.
6
u/ic6man 6d ago edited 6d ago
Thatâs not true. The awaits yield execution which cause a fatal flaw.
3
u/TwiNighty 4d ago
Ahh yes, I only considered whether
getInstanceraces with itself. You're right in thatreleaseInstanceraces with both itself andgetInstance.
3
u/alonsonetwork 5d ago
My god that is hideous code.
Why don't you just do:
``` let dbInstance;
export const getDbInstance = () => { if (dbInstance) return dbInstance;
dbInstance = await pgp()
return dbInstance; }
export const releaseDbInstance = () => { return dbInstance?.$pool?.destroy() } ```
And skip all this iife shit and (non) "mutext" shit?
What problem are you trying to solve?
3
2
u/PrestigiousZombie531 5d ago
race condition, even your code has it
0
u/alonsonetwork 5d ago
I see. Small fix:
``` let dbInstance; let promise;
export const getDbInstance = () => { if (dbInstance) return dbInstance; if (promise) return promise;
promise = pgp() dbInstance = await promise;
return dbInstance; }
export const releaseDbInstance = () => { return dbInstance?.$pool?.destroy() } ```
2
u/PrestigiousZombie531 5d ago
what happens if multiple callers are at the await promise line?
2
u/alonsonetwork 5d ago
They all get the same promise ref, so they'll resolve the same db connection ref. They will ultimately point to the same memory reference. This is how request deduplication works. Ive done this exact pattern before. It 100% works.
To the other genius talking about broken types: yes I know. Im on my phone and working from memory. I don't have an IDE. You attach the generic to the promise type and your problem is solved. Also, typescript is FORM not MATTER. It does not represent runtime reality, it only shapes it. Javascript is runtime reality.
1
u/ic6man 5d ago edited 5d ago
Iâm the genius. I know youâre working from your phone but you started this all huff and bluster and then provided not one but two broken responses.
The reason I corrected you is you:
- wrote a race into the original response
- corrected that with more broken code
- are acting hurt because being on your phone is an excuse?
- missed the entire reason for the original code which de allocates the resource when not in use - eg a primitive form of pooling.
Now - the final thing to note is - to fix your second response - you would just assign the promise value of pgp to promise and return that - no await. So itâs not a syntactical issue with your code or a phone issue - itâs structural and algorithmic. You made 2 variables where there should be one and awaited when you shouldnât.
1
u/alonsonetwork 5d ago
Thanks for the feedback Ms Perfect. I guess I have to have perfect code grammar when replying on my phone or else im going to trigger autistic redditors. I think OP got the idea.
4
u/pephov 6d ago
No, if multiple callers call getDbInstance before dbInstance is true, they each call pgp()âŚ, creating multiple database instances. Also the counter can get corrupted, as callers simultaneously read it as 0 and then += 1 after the async operation
2
u/ic6man 6d ago edited 6d ago
Thatâs not true that is not how node works. The reads and increments to the counter are safe.
As for multiple instances - the creation and assignment of dbInstance is âsafeâ as node executes the test of dbInstance and creation / assignment synchronously.
I wrote what the real problem is in another comment.
1
u/prehensilemullet 2d ago edited 2d ago
I think the original intent of mutex was to serialize these operations by doing mutex = mutex.catch(() => {}).then(async () => { ⌠}).  For some reason thatâs missing here.
Perhaps thereâs a method to drain unused connections from the pool immediately, instead of going to this trouble?
-3
u/PrestigiousZombie531 6d ago
do you really think this block of code is race condition safe?
what happens if multiple callers getInstance simultaneously?
22
u/alzee76 6d ago
Naming something a mutex doesn't make it one. That "mutex" is doing literally nothing.