GH-4968: server-spring - Close query result on pre-render exception #4969
Conversation
| queryResponse.close(); | ||
| } | ||
| } catch (Exception qre) { | ||
| logger.warn("Query response closing error", qre); |
There was a problem hiding this comment.
Question: I thought it might be sensible here to log the result-close failure so the client still receives the original failure reason. Is that the right approach? And: Would it make sense to do the same for repositoryCon below?
|
It looks like the integration test failure is unrelated (in that this from my understanding doesn't use |
|
I'm rerunning that GitHub action to see if it is just temperamental. I haven't seen it being flaky before though. |
| // only close the response & connection when an exception occurs. Otherwise, the QueryResultView will take | ||
| // care of closing it. | ||
| try { | ||
| if (queryResponse != null) { |
There was a problem hiding this comment.
Maybe you can simplify your code down to an instanceOf AutoClosable check here without having to introduce the Boolean query stuff.
There was a problem hiding this comment.
fb01e2a
(to be squashed before merge)
|
Test is still failing. I suspect it's related to the Boolean query changes you've introduced. I added a proposal to your code for a simple instanceOf AutoCloseable check instead. |
|
I just realised that I'm probably doing the whole "add more commits during review" thing wrong: Internally we use What's your preference?
|
|
We typically rebase and squash locally then force push the branch. When we merge here on GitHub we try to only use regular merge since either of squash/rebase changes the author on the commits. If you can squash your commits into one and force push it I’d be grateful. |
…r exception - Previously between getting the query response and rendering it to the desired output an exception could occur (e.g. due to unsupported result format) which would leave the result un-closed. This in turn would sometimes trigger an exception on closing the associated repo connection.
vtermanis
force-pushed
the
GH-4968-server-spring-close-result
branch
from
b365276
to
f9ed099
Compare
|
Squashed |
|
Ah wait - should the target branch also be |
|
Since this was now a pure bug fix I went ahead and merged it into main instead of develop. We regularly merge main into develop to keep things in sync. |
GitHub issue resolved: #4968
Briefly describe the changes proposed in this PR:
AbstractQueryRequestHandlerclass.PR Author Checklist (see the contributor guidelines for more details):
mvn process-resourcesto format from the command line)