nodejs / undici

An HTTP/1.1 client, written from scratch for Node.js
https://nodejs.github.io/undici
MIT License
6.06k stars 528 forks source link

fetch: wpt tests that are failing #1666

Open KhafraDev opened 1 year ago

KhafraDev commented 1 year ago

We check if signal is aborted here: https://github.com/nodejs/undici/blob/9c3f34c97dfce7f7f6cffd04b7d4dab9921dc40d/lib/fetch/index.js#L126 and the promise gets rejected here: https://github.com/nodejs/undici/blob/9c3f34c97dfce7f7f6cffd04b7d4dab9921dc40d/lib/fetch/index.js#L304

import { fetch, setGlobalOrigin } from 'undici'
import assert from 'assert'

setGlobalOrigin('http://localhost:3000')

const controller = new AbortController();
const signal = controller.signal;
controller.abort();

const log = [];

await Promise.all([
    fetch('../resources/data.json', { signal }).then(
        () => assert_unreached("Fetch must not resolve"),
        () => log.push('fetch-reject')
    ),
    Promise.resolve().then(() => log.push('next-microtask'))
]);

assert.deepStrictEqual(log, ['fetch-reject', 'next-microtask']);

Originally posted by @KhafraDev in https://github.com/nodejs/undici/issues/1664#issuecomment-1256836575

KhafraDev commented 1 year ago

This case also fails:

const request = new Request('https://example.com', { method: 'POST', body: 'body' })
const originalBody = request.body
assert(request.bodyUsed === false)

const otherRequest = new Request(request, { body: 'another body' })
assert(request.bodyUsed === true) // <--- this assert fails
assert(request.body === originalBody)
assert(originalBody !== undefined)
assert(originalBody !== null)
assert(otherRequest.body !== originalBody)
KhafraDev commented 1 year ago

This case fails:

const body = new ReadableStream()

assert.throws(() => {
  new Request('https://example.com', { method: 'POST', body })
}, TypeError)

edit: this is a bug in the test itself, which doesn't specify duplex: 'half'.

KhafraDev commented 1 year ago
const initialBuffer = new Int8Array(new ArrayBuffer(16))

const stream = new ReadableStream({
  start (controller) {
    controller.enqueue(initialBuffer)
    controller.close()
  }
})
const [out1, out2] = stream.tee()

const { value } = await out2.getReader().read()

console.log(value === initialBuffer) // this should be false

The same subset of tests are failing here (https://staging.wpt.fyi/results/fetch/api/response/response-clone.any.html?label=master&label=experimental&aligned&view=subtest)

The spec makes no mention of cloning the value returned by ReadableStreamDefaultReader.read that I could find. It's likely why this test is failing in most other environments.

KhafraDev commented 1 year ago

Will be fixed by #1697

https://github.com/web-platform-tests/wpt/blob/master/fetch/api/response/response-clone.any.js

These tests are mostly/all failing due to us not cloning the tee'd ReadableStream from response.clone()

A fix would look like:

const [out1, out2] = stream.tee()

const cloned = structuredClone(out2, { transfer: [out2] })
KhafraDev commented 1 year ago

Another test fails due to fetch allowing binding of this:

Expected outcomes:

const url = 'https://example.com'

await fetch.call({}, url) // rejects
await fetch.call(undefined, url) // resolves
await fetch.call(globalThis, url) // resolves

The problem is that depending on how undici is imported, the default this can be a few values:

  1. undefined
  2. module
  3. module.exports
  4. some sort of object used when importing; we don't have access to it to compare.

We can't bind the value of this because when logging fetch it would should [Function bound fetch] and, currently, this is already bound to the dispatcher being used in the request.

Imports expected to work:

  1. const { fetch } = require('undici'); fetch(...)
  2. const undici = require('undici'); undici.fetch(...)
  3. import * as undici from 'undici'; undici.fetch(...)
  4. import undici from 'undici'; undici.fetch(...)
  5. import { fetch } from 'undici'; fetch(...)
mcollina commented 1 year ago

Should we create fetch as a closure instead?

KhafraDev commented 1 year ago

I will experiment with it, but none of my initial attempts got anywhere