fix: Properly reseve connection for the duration of the request #7
Conversation
Problem: connection pool does not properly reserve connection when it is in use. So, it is possible that two requests within the same session will start executing in parallel. Mysql driver is not thread-safe and this will cause malformed network packed. Fixed by attaching extra mutex to each connection, that would be locked for duration of the request.
There was a problem hiding this comment.
The code as it is is good enough, specially given that a) solves our problem, b) PS shown interest in making improvements to the proxy based on our feedback.
An alternative, more idiomatic design for the linearization of queries over a connection would be to have a channel to process requests and one to write responses. A non-tested implementation can be found in https://gist.github.com/miguelff/0e49db62c80780f0a22d77f86f7d70fc
Still it might need to be adapted to work with the pool. A complete refactoring might be possible but that's out of the scope of this fix. To reiterate. Your implementation is good enough and solves the problem we are having! so 💯
| if err != nil { | ||
| if strings.Contains(err.Error(), "Access denied for user") { | ||
| return nil, connect.NewError(connect.CodeUnauthenticated, err) | ||
| } | ||
| return nil, err | ||
| } | ||
| defer unlockConn(creds.Username(), pass, session) |
There was a problem hiding this comment.
It's difficult to reason about this defer call in the middle of the method, if it's possible to inline it , readability will improve; also if ExecuteFetch wouldn't panic, better to put the defer block at the end of the function to let the function express more straightforward what happens when.
|
My feedback here: mattrobenolt#1 We concluded that it'd make more sense for the proxy to explicitly disallow this behavior, since anytime this behavior happens, it'd lead to undefined behavior and should be considered a bug in the client or driver. A single session is not expected to be used in parallel, and having this simulator gets us an opportunity to actually error here and message that, instead of forcing some nondeterministic serialized order. To be clear, for Prisma's context. Our "session" is most similar to a browser cookie, but represents a single MySQL TCP connection. While you can multiplex multiple sessions over the same TCP/HTTP stream to us, a single "session" must not be multiplexed. We don't explicitly disallow this, but ultimately, like sorta proposed here, the operations did get serialized down to mysqld over a single connection. I strongly discourage attempting to do that since a new session is free over HTTP, so they can be used more liberally anytime concurrency is needed. So arguably, this is more a bug in the Prisma driver that my bug exposed. |
|
Thank you @mattrobenolt, we agree that this is more of our implementation bug and will do necessary changes to our adapter instead. |
|
I've dropped a new build: https://github.com/mattrobenolt/ps-http-sim/releases/tag/v0.0.2 |
Problem: connection pool does not properly reserve connection when it is
in use. So, it is possible that two requests within the same session
will start executing in parallel. Mysql driver is not thread-safe and
this will cause malformed network packed.
Fixed by attaching extra mutex to each connection, that would be locked
for duration of the request.
Fix prisma/team-orm#494
Fix prisma/team-orm#540
Contributes to prisma/team-orm#518