fix iterating headers with `set-cookie` #4048

dylan-conway · 2023-08-07T22:26:52Z

What does this PR do?

before:

const headers = new Headers();
headers.append("Set-Cookie", "cookies");
headers.append("Content-Type", "application/json");

console.log([...headers]);
// output: [ [ "content-type", "application/json" ], [ "content-type", "application/json" ] ]

after:

// output: [ [ "content-type", "application/json" ], [ "set-cookie", "cookies" ] ]

How did you verify your code works?

updated and added test for headers with set-cookie

paperclover · 2023-08-07T22:40:58Z

~~This will close #600 :)~~ I mispoke, it will only close it for bun --bun run dev

There is a bug in building that still has to get investigated.

github-actions · 2023-08-07T22:51:44Z

❌ @dylan-conway 4 files with test failures on bun-darwin-aarch64:

test/bundler/esbuild/default.test.ts
test/cli/install/bun-install.test.ts
test/js/bun/test/test-test.test.ts
test/regression/issue/4011.test.ts

View test output

^{#4c17d384f6ffb9b69058ad3f7cb8f7bc83392ad0}

github-actions · 2023-08-07T22:55:05Z

❌ @dylan-conway 2 files with test failures on linux-x64-baseline:

test/js/bun/test/test-test.test.ts
test/js/third_party/prisma/prisma.test.ts

View test output

^{#4c17d384f6ffb9b69058ad3f7cb8f7bc83392ad0}

github-actions · 2023-08-07T22:56:10Z

❌ @dylan-conway 3 files with test failures on linux-x64:

test/js/bun/test/test-test.test.ts
test/js/third_party/prisma/prisma.test.ts
test/js/web/fetch/fetch.test.ts

View test output

^{#4c17d384f6ffb9b69058ad3f7cb8f7bc83392ad0}

github-actions · 2023-08-07T23:05:56Z

❌ @dylan-conway 7 files with test failures on bun-darwin-x64-baseline:

test/js/bun/spawn/spawn-streaming-stdin.test.ts
test/js/bun/spawn/spawn-streaming-stdout.test.ts
test/js/bun/sqlite/sqlite.test.js
test/js/bun/test/test-test.test.ts
test/js/third_party/webpack/webpack.test.ts
test/js/web/timers/setInterval.test.js
test/js/web/timers/setTimeout.test.js

View test output

^{#4c17d384f6ffb9b69058ad3f7cb8f7bc83392ad0}

Jarred-Sumner · 2023-08-08T00:53:13Z

src/bun.js/bindings/webcore/FetchHeaders.cpp

        for (auto& header : m_headers->m_headers)
            m_keys.uncheckedAppend(header.asciiLowerCaseName());
-        std::sort(m_keys.begin(), m_keys.end(), WTF::codePointCompareLessThan);
+        if (hasSetCookie)


Do we need to pay this performance overhead here? Wouldn't it be fine if we put the Set-Cookie header either first or last?

we do, this is part of the fetch spec https://fetch.spec.whatwg.org/#convert-header-names-to-a-sorted-lowercase-set

It was also expected in the spec for keys to be unique which isn’t true for set-cookies

I think we should just always put it at the end and call it a day. Each call to next() shouldn’t slow it down

updated to append after the sort

src/bun.js/bindings/webcore/FetchHeaders.cpp

Jarred-Sumner

These changes should be moved to HTTPHeaderMap to avoid duplicate work

src/bun.js/bindings/webcore/FetchHeaders.cpp

src/bun.js/bindings/webcore/HTTPHeaderMap.cpp

src/bun.js/bindings/webcore/FetchHeaders.cpp

Jarred-Sumner · 2023-08-08T02:32:31Z

Thank you

* fix iterating headers with `set-cookie` * a test * move work to `HTTPHeaderMap::set` * append set-cookie after sort * remove compare function

dylan-conway added 2 commits August 7, 2023 15:17

fix iterating headers with set-cookie

6dc5035

a test

4110307