-
Notifications
You must be signed in to change notification settings - Fork 441
fix: prevent stale errors #1717
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
arthurschreiber
wants to merge
5
commits into
master
Choose a base branch
from
claude/investigate-tedious-1712-NSvDL
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
e6106fe
fix: reset request.error in makeRequest() to prevent stale errors (#1…
claude be99e06
refactor: use async/await pattern in GH#1712 test
claude 424bcd8
refactor: use try/catch pattern for error handling in test
claude b58f128
revert: use callback-based test for GH#1712
claude ff14d7d
fix: wait for unprepare() to complete before closing connection
github-actions[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling
close()without waiting for theunprepare()to complete cancels theunprepare()immediatelly so that is has no effect. While unprepare is not necessary before a close anyway, the test case is not entirely correct in this form and it's a bad example for the users.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@claude Can you fix this?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude finished @arthurschreiber's task in 1m 27s —— View job
Fixed test to wait for unprepare() before close()
I've fixed the issue where
close()was called without waiting forunprepare()to complete.Todo List:
Changes made:
test/integration/prepare-execute-statements-test.ts:199-202to listen for therequestCompletedevent before callingclose()unprepare()completes before the connection is closedThe fix follows the same event-driven pattern used throughout the test file, where operations are chained using
requestCompletedevent handlers. The test now properly waits for unprepare to finish before closing, making it a correct example for users.Commit:
ff14d7dThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chdh Claude fixed this, but looking at how the prepare/unprepare API is supposed to be used, it makes very little sense to me. 😟
Looks like the callback to
new Requestis called when the request is unprepared? But how is the user supposed to understand the difference between a request being unprepared and a request being executed?This seems to be a major design flaw in the current API -
prepare,execute,unprepare,execSqlBatch,execSqlall should take their own callbacks, andnew Requestshould not even have a callback set.Also I guess there should be a
PreparedRequestobject that's returned byprepareand that can be passed intoexecuteandunprepare.Or do I misunderstand the current API? Do you think the "future" API I'm laying out above is not going in the right direction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably
execSql,executeandexecSqlBatchshould actually return aResponseobject which can be iterated over / events will be emitted on.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I had exactly this problem when I wrote my promise-based wrapper. I solved it by routing the request completion callback through a function pointer in a
PreparedStatementobject.