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

Broken response streams #901

Merged
merged 9 commits into from
Dec 7, 2024
Merged

Broken response streams #901

merged 9 commits into from
Dec 7, 2024

Conversation

drewvolz
Copy link
Member

@drewvolz drewvolz commented Nov 25, 2024

Response objects in ccc are being consumed more than once without being cloned first. If we are using the response body multiple times, which is a stream, it can't be read again, so we have issue.

* denotes we have seen it error, check means this PR changes that

  • A-to-z *
  • Calendar
  • Contacts *
  • Dictionary *
  • FAQs *
  • Hours
  • Jobs
  • Menu * (in our smoke test)
  • News
  • Orgs
  • Routes [dev only] *
  • Streams *
  • Transport

$ docker-compose logs

TypeError: Response.clone: Body has already been consumed.
    at webidl.errors.exception (node:internal/deps/undici/undici:3384:14)
    at _Response.clone (node:internal/deps/undici/undici:8883:31)
    at result.<computed> [as json] (file:///app/node_modules/ky/distribution/core/Ky.js:55:48)
    at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
    at async getOlafAtoZ (file:///app/dist/source/ccci-stolaf-college/v1/a-z.js:23:41)
    at async atoz (file:///app/dist/source/ccci-stolaf-college/v1/a-z.js:49:24)
    at async bodyParser (/app/node_modules/koa-bodyparser/index.js:78:5)
    at async etag (/app/node_modules/koa-etag/index.js:27:5)
    at async /app/node_modules/koa-conditional-get/index.js:15:5
    at async compressMiddleware (/app/node_modules/koa-compress/lib/index.js:56:5)

The assumption being that this could be triggered by another request, logging, middleware, or most likely our mem caching.

The issue being the memoized response will return the same Response object, leading to an error if we are trying to access something on it.

This is what I am thinking is happening

  1. GET(url) makes the http request and returns a Response object
  2. .json() consumes the body of the Response
  3. Memoized GET(url) --> subsequent calls to GET(url).json() reuse the same Response object from first http call

I can point my physical phone at a local version of ccc but I haven't replicated the issue when running local ccc so far.


Separately, when enabling sentry logging locally by adding the DSN env var, the following was logged

[Sentry] koa is not instrumented. This is likely because you required/imported koa before calling Sentry.init().

@drewvolz
Copy link
Member Author

Our smoke test seems to have caught one of these!

validating /v1/food/named/menu/burton
  <-- GET /v1/food/named/menu/burton
  xxx GET /v1/food/named/menu/burton 500 1ms -

  TypeError: Response.clone: Body has already been consumed.
      at webidl.errors.exception (node:internal/deps/undici/undici:3564:14)
      at _Response.clone (node:internal/deps/undici/undici:9081:31)
      at result.<computed> [as text] (file:///home/runner/work/ccc-server/ccc-server/node_modules/ky/distribution/core/Ky.js:55:48)
      at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
      at async getBonAppWebpage (file:///home/runner/work/ccc-server/ccc-server/dist/source/menus-bonapp/index.js:23:18)
      at async _menu (file:///home/runner/work/ccc-server/ccc-server/dist/source/menus-bonapp/index.js:70:15)
      at async burtonMenu (file:///home/runner/work/ccc-server/ccc-server/dist/source/ccci-stolaf-college/v1/menu.js:97:16)
      at async bodyParser (/home/runner/work/ccc-server/ccc-server/node_modules/koa-bodyparser/index.js:78:5)
      at async etag (/home/runner/work/ccc-server/ccc-server/node_modules/koa-etag/index.js:27:5)
      at async /home/runner/work/ccc-server/ccc-server/node_modules/koa-conditional-get/index.js:15:5

@drewvolz drewvolz changed the title Fix response streams Broken response streams Nov 25, 2024
@drewvolz drewvolz merged commit 17c9ee7 into master Dec 7, 2024
7 of 9 checks passed
@drewvolz drewvolz deleted the drew/fix-response-streams branch December 7, 2024 04:33
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 this pull request may close these issues.

2 participants