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

Call log entries may cause response body reading to hang #575

Closed
ProTip opened this issue Jun 22, 2020 · 4 comments · Fixed by #580
Closed

Call log entries may cause response body reading to hang #575

ProTip opened this issue Jun 22, 2020 · 4 comments · Fixed by #580

Comments

@ProTip
Copy link
Contributor

ProTip commented Jun 22, 2020

This is related to a PR that I myself submitted and was merged recently: #571 .

After submitting this I started working on my own small interception system to record and play back(through fetch-mock actually) API calls. I ran into an issue where if the cloned response and the original response bodies were not both being asynchronously read, at least with node-fetch, the one being read may never resolve.

This is due to the underlying stream implementation and the node-fetch recommendation is to consume them both: node-fetch/node-fetch#665 (comment) .

In my implementation I had to store a log of Promises that consume the body and resolve to a record. I would recommend either reverting my change or storing a such a promise in the call record to prevent the streams hanging..

@wheresrhys
Copy link
Owner

Oh man - this is horrible. I've just been hit by it

@wheresrhys
Copy link
Owner

I think the right approach to this is to print a warning whenever the following two conditions apply:

  1. the environment the test is running in is a node process
  2. fetch-mock clones a response

At heart the feature is a good one and it's a powerful addition to the library that executes perfectly well in other environments. I don't feel that removing it is right.

Any comment on this approach @ProTip ?

@wheresrhys
Copy link
Owner

Actually I think I have a better fix - cloning on demand when inspecting the call logs rather than peremptively when creating them (though I think printing a warning is still necessary)

@wheresrhys
Copy link
Owner

wheresrhys commented Jul 19, 2020

Mitigated in v9.10.4

Give your interception system another shot - hopefully it will work now

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

Successfully merging a pull request may close this issue.

2 participants