Skip to content
This repository has been archived by the owner on Jan 27, 2022. It is now read-only.

handle external fetch abort #880

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

rezof
Copy link
Contributor

@rezof rezof commented Nov 29, 2018

  • fixes issue
  • test already covered, but i can write a specific test for it if needed

TODO:

  • Make sure all of new logic is covered by tests and passes linting
  • Update CHANGELOG.md with your change

@apollo-cla
Copy link

@rezof: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@rezof rezof changed the title handle direct fetch abort handle external fetch abort Dec 13, 2018
@codecov-io
Copy link

Codecov Report

Merging #880 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #880   +/-   ##
=======================================
  Coverage   95.21%   95.21%           
=======================================
  Files          22       22           
  Lines        1024     1024           
  Branches      109      109           
=======================================
  Hits          975      975           
  Misses         44       44           
  Partials        5        5
Impacted Files Coverage Δ
packages/apollo-link-http/src/httpLink.ts 100% <100%> (ø) ⬆️

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 243c080...215cbad. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Feb 11, 2019

Codecov Report

Merging #880 into master will decrease coverage by 0.05%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #880      +/-   ##
==========================================
- Coverage   95.19%   95.14%   -0.06%     
==========================================
  Files          22       22              
  Lines        1020     1029       +9     
  Branches      103      103              
==========================================
+ Hits          971      979       +8     
- Misses         44       45       +1     
  Partials        5        5
Impacted Files Coverage Δ
packages/apollo-link-http/src/httpLink.ts 100% <100%> (ø) ⬆️
.../apollo-link-http/src/__tests__/sharedHttpTests.ts 93.82% <88.88%> (-0.11%) ⬇️

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 8e0ef32...eeb1d52. Read the comment docs.

@codecov-io
Copy link

Codecov Report

Merging #880 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #880   +/-   ##
=======================================
  Coverage   95.21%   95.21%           
=======================================
  Files          22       22           
  Lines        1024     1024           
  Branches      109      109           
=======================================
  Hits          975      975           
  Misses         44       44           
  Partials        5        5
Impacted Files Coverage Δ
packages/apollo-link-http/src/httpLink.ts 100% <100%> (ø) ⬆️

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 243c080...215cbad. Read the comment docs.

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @rezof! While this change looks promising and makes sense, it would be great if you could add a test that shows this fixes apollographql/apollo-client#4150. That way we can make sure this is accounted for in future releases. Thanks again!

@jmadelaine
Copy link

I've run into this issue. When will this change be implemented as it is holding my project back?

@JoviDeCroock
Copy link
Contributor

JoviDeCroock commented Feb 17, 2019

I've run into this issue. When will this change be implemented as it is holding my project back?

When this test is implemented it will probably be pushed forward. If OP doesn't feel up to this, I'll try.

@rezof
Copy link
Contributor Author

rezof commented Feb 20, 2019

@hwillson @JoviDeCroock test added

@Optarion
Copy link

Any news on this?

// fetch was cancelled so its already been cleaned up in the unsubscribe
if (err.name === 'AbortError') return;
// exit if fetch was cancelled from the observer unsubscribing
if (err.name === 'AbortError' && observer.closed) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Think you don't even need to check what type of error it is. If nobody is listening (observer.closed), you can return here.

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, you shouldn't care what type of error it is. As long as someone is subscribed, they need to find out about this error, otherwise you're just leaving them hanging.

PS: As you probably figured out, the query deduplicator never finding out that the fetch was aborted is what's causing subsequent requests to hang.

@helfer
Copy link
Contributor

helfer commented Apr 28, 2019

FWIW, I think this PR makes sense, but it isn't a solution to the original issue. Using an AbortController to reach into the link stack and abort a request instead of unsubscribing breaks the abstraction. If you continue trying to work around it by putting in ifs and elses here and there, the code will become bloated and much more complicated, and you'll likely introduce more bugs and corner-cases along the way.

I think the most logical way to go about this would be to make the promise returned by client.query cancelable (if it isn't already), and then propagating that cancellation up the stack. Presumably this is already what happens with watchQuery anyway when you unsubscribe?

@mo
Copy link

mo commented Jul 2, 2020

It would be nice of useLazyQuery() returned a "cancel" function.

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

Successfully merging this pull request may close these issues.

if a Query is stopped using AbortController, it cannot be executed again.
9 participants