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

Add aborting to CoreGetFeatures rpcManager call #1874

Merged
merged 7 commits into from
Aug 13, 2021

Conversation

cmdcolin
Copy link
Collaborator

@cmdcolin cmdcolin commented Apr 4, 2021

This PR that adds an AbortSignal options parameter to "CoreGetFeatures" rpcManager

This was just based on the observation that "SequenceDialog" if a large region is selected and then the dialog is closed, does not cancel the fetch of sequence

This does not yet actually add full aborting for that sequence fetch, because the sequence adapters such as indexedfasta-js et al would have to add abort support to make this really work, but it may be worth it to have the infrastructure added here anyways

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Apr 4, 2021
@cmdcolin cmdcolin force-pushed the add_coregetfeatures_aborting branch from d92ebdd to 7975c87 Compare April 4, 2021 15:30
@codecov
Copy link

codecov bot commented Apr 4, 2021

Codecov Report

Merging #1874 (e292d98) into main (b5cb5ab) will increase coverage by 0.00%.
The diff coverage is 54.54%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1874   +/-   ##
=======================================
  Coverage   62.56%   62.56%           
=======================================
  Files         484      484           
  Lines       22707    22711    +4     
  Branches     5152     5153    +1     
=======================================
+ Hits        14206    14210    +4     
  Misses       8231     8231           
  Partials      270      270           
Impacted Files Coverage Δ
...src/LinearGenomeView/components/SequenceDialog.tsx 55.69% <46.15%> (+5.02%) ⬆️
...nce/src/IndexedFastaAdapter/IndexedFastaAdapter.ts 75.67% <50.00%> (+0.67%) ⬆️
packages/core/rpc/coreRpcMethods.ts 78.40% <85.71%> (-0.25%) ⬇️
...equence/src/BgzipFastaAdapter/BgzipFastaAdapter.ts 76.92% <100.00%> (ø)
packages/core/util/io/rangeFetcher.ts 90.24% <0.00%> (-2.44%) ⬇️
packages/core/assemblyManager/assemblyManager.ts 60.00% <0.00%> (-1.43%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5cb5ab...e292d98. Read the comment docs.

@cmdcolin cmdcolin added internal and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Apr 4, 2021
@cmdcolin cmdcolin force-pushed the add_coregetfeatures_aborting branch from 0e33521 to dadca07 Compare April 5, 2021 18:04
@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Apr 5, 2021

@garrettjstevens any opinion on the opts parameter being explicitly separate? IIRC this is not how some of our other RPC work, and instead the entire args gets passed in as opts

@garrettjstevens
Copy link
Collaborator

@cmdcolin Good point, now that I think about it, signal should be directly in args and not args.opts since RpcMethodType.deserializeArguments(args) does some work to deserialize args.signal.

@cmdcolin cmdcolin marked this pull request as draft April 5, 2021 19:51
@cmdcolin cmdcolin force-pushed the add_coregetfeatures_aborting branch from dadca07 to cd696ed Compare May 28, 2021 01:13
@cmdcolin cmdcolin force-pushed the add_coregetfeatures_aborting branch from cd696ed to 72a41ae Compare July 15, 2021 15:49
@cmdcolin
Copy link
Collaborator Author

Tried adding GMOD/indexedfasta-js#57 but it did not yet achieve aborting when closing the dialog. Aborting is hard as usual

@cmdcolin cmdcolin force-pushed the add_coregetfeatures_aborting branch from ad32d85 to dd6b510 Compare July 27, 2021 16:47
@cmdcolin cmdcolin force-pushed the add_coregetfeatures_aborting branch from dd6b510 to b5c5ba0 Compare August 10, 2021 14:42
@cmdcolin cmdcolin marked this pull request as ready for review August 10, 2021 14:57
@cmdcolin cmdcolin merged commit 14f5263 into main Aug 13, 2021
@cmdcolin cmdcolin deleted the add_coregetfeatures_aborting branch August 13, 2021 18:55
@cmdcolin
Copy link
Collaborator Author

note that this PR did not achieve actually performing an abort on closing the dialog, but just as a step in the right direction, just went ahead with merge :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants