Fix error handling for exceptions on values parsing.#3574
Fix error handling for exceptions on values parsing.#3574APshenkin wants to merge 2 commits intobrianc:masterfrom
Conversation
|
I would really appreciate to get a feedback first, that those changes are in right direction. Those two changes are tight together as they both are related to serialization issue. Just because I solved global writer issue first, zombie client appeared next in PgPool, because without fixing writers you just write sync/close in next query I'm not an expert of pg protocol and just investigated this issue few hours. |
|
Yes, these changes make sense. The shared writer was probably an attempt at improving performance. |
If this is something which can be acomplished without reinstantiating the writer I would greatly prefer it. This is extremely hot path code. Is there a way to fix this w/o? Also: needs tests before merging. Thanks for the eyes on this tho! I'll leave it open for a bit as it might be something I can quickly fix (IF there is a repoducable test case on how to trigger the issue) |
|
Sorry, I missed these comments, my bad. I'll try to take a look on this back in a next few weeks (split to different commits, adding tests) Re
I think it should be doable without reinstantiating the writer, however the fix will be fragile then for any future changes. Also code will be a bit ugly (each time when you have some error you should not forget to clean it properly). I'll try to play around it and come back with something |
sounds good - i think we can just re-instantiate it then. allocating something is pretty cheap. |
|
Is it possible to write a couple tests for this? I'd like to get it merged. |
…er in bind() When valueMapper throws during serialize.bind(), the module-level singleton Writer instances (writer and paramWriter) are left with partial data from the interrupted operation. This corrupts all subsequent serializer calls since they share the same Writer instances. Add Writer.clear() method that resets the write cursor without allocating a new buffer (zero overhead on the happy path). Wrap writeValues() in bind() with try-catch to clear both writers on error.
When connection.bind() throws during prepare(), the catch block previously called handleError() without sending any protocol messages. Since PARSE was already buffered (due to cork), the server processes it and waits for more messages, leaving the connection hung. Send Close to clean up the parsed statement and Sync to return the connection to a usable state.
APshenkin
force-pushed
the
fix/fix-params-parsing-exceptions
branch
from
7c6914f to
0e0ff2a
Compare
|
I implemented change in a way to not reinstantiate writers, added tests for both issues and split two fixes into two different commits. Let me know if that will work for you |
3 participants
Fixes #3573
Writerinstantiation to improve isolation and prevent state reuse issues.