Skip to content
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

ParallelQueryExecutionContextBase breaks use of abortSignal #18544

Merged
merged 3 commits into from
Apr 6, 2022

Conversation

jay-most
Copy link
Contributor

@jay-most jay-most commented Nov 4, 2021

@azure/cosmos:
3.14.1:
Describe the bug
If an abortSignal object is passed within the FeedOptions object of a query AND
a query rewrite response is initially received by the SDK, an exception is thrown:

userSignal.addEventListener is not a function
The issue is within the function ParallelQueryExecutionContextBase._createTargetPartitionQueryExecutionContext which is attempting to make a deep copy of the query options using JSON.parse(JSON.stringify(...)). This will copy object data but will not copy the abortSignal object member functions which are required to add event listeners for abort triggers.

I recommend the following change for line 474 of azure-sdk-for-js/sdk/cosmosdb/cosmos/src/queryExecutionContext/parallelQueryExecutionContextBase.ts

  •    const options = JSON.parse(JSON.stringify(this.options));
    
  •    const options = Object.assign(Object.create(Object.getPrototypeOf(this.options)), this.options);
    

To Reproduce
Steps to reproduce the behavior:

Query a container for items options.abortSignal defined and a SqlQuerySpec that will trigger a query rewrite.
const abortController = new AbortController();
const querySpec: SqlQuerySpec = {
query: "SELECT MAX(c._ts) as _ts, c.someAttrib FROM c WHERE IS_DEFINED(c.someAttrib) GROUP BY c.someAttrib",
parameters: [
]
};
const options: FeedOptions = {
abortSignal: abortController.signal
};
const iterator = container.items.query(querySpec, options);
Expected behavior
No exception thrown.

Copy link
Member

@kushagraThapar kushagraThapar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me, thanks @jay-most

@ghost ghost added the no-recent-activity There has been no recent activity on this issue. label Feb 25, 2022
@ghost
Copy link

ghost commented Feb 25, 2022

Hi @jay-most. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@ghost ghost closed this Mar 4, 2022
@ghost
Copy link

ghost commented Mar 4, 2022

Hi @jay-most. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing "/reopen" if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the "no-recent-activity" label; otherwise, this is likely to be closed again with the next cleanup pass.

@jay-most jay-most reopened this Mar 9, 2022
@jay-most
Copy link
Contributor Author

jay-most commented Mar 9, 2022

@kushagraThapar

@ghost ghost removed the no-recent-activity There has been no recent activity on this issue. label Mar 9, 2022
@jay-most jay-most marked this pull request as ready for review March 9, 2022 01:08
@jay-most jay-most merged commit 9ff8ad5 into main Apr 6, 2022
@jay-most jay-most deleted the parallel_queryexecutioncontext_base_abort_signal branch April 6, 2022 15:45
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-js that referenced this pull request Apr 14, 2022
Review request for Microsoft.ContainerService to add version 2022-03-02-preview (Azure#18633)

* Adds base for updating Microsoft.ContainerService from version preview/2022-02-02-preview to version 2022-03-02-preview

* Updates readme

* Updates API version in new specs and examples

* feat: add `/rotateServiceAccountSigningKeys` API (Azure#18359)

* feat: add `/rotateServiceAccountSigningKeys` API

* chore: remove duplicated description

* feat: add `workloadIdentity` settings to `SecurityProfile` (Azure#18360)

* fix: update example name (Azure#18382)

* add creationData to mc data (Azure#18414)

* add creationData to mc data

* fix test

* fix format

* Add API properties and example JSON for Web App Routing of IngressProfile (Azure#18564)

* Add API properties and example JSON for Web App Routing of IngressProfile.

* Add a ending period for description to match the style in all other "descriptions" in the same file.

* Update readmes for the 2022-03-02-preview dev branch of container service (Azure#18358)

* update readme

* update sdk readmes

* Agentpool alias minor version 2022-03-02-preview (Azure#18381)

* Add field currentOrchestratorVersion to support Agentpool alias minor version

* Add new exmaple for alias minor version

* Remove example for PrettierCheck

* Fix typo

* Latest patch version supported is 1.22.7 at the moment

* Address comments - refine descriptions for fields

* feat: add ManagedCluster StorageProfile in 0302preview (Azure#18590)

Signed-off-by: Ji An Liu <[email protected]>

* Swagger change for ignore-pod-disruption-budget (Azure#18548)

* Swagger change for ignore-pod-disruption-budget

* Change ignorePodDisruptionBudget to string in example file.

* Change ignorePodDisruptionBudget to boolean type.

* add effectiveNoProxy for AKS (Azure#18544)

* add effectiveNoProxy for AKS

* add to correct api

* fix lowercase -> uppercase O

* spelling

* Replace common type definitions with references since 2022-03-02-preview for Microsoft.ContainerService  (Azure#18567)

* replace Resource

* replace SystemData

* replace parameters

* change track2 to python

Co-authored-by: hbc <[email protected]>
Co-authored-by: Qingqing <[email protected]>
Co-authored-by: Yi Zhang <[email protected]>
Co-authored-by: Thalia Wang <[email protected]>
Co-authored-by: Ji'an Liu <[email protected]>
Co-authored-by: Tong Chen <[email protected]>
Co-authored-by: Ace Eldeib <[email protected]>
@jay-most jay-most mentioned this pull request Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants