Don't send the Set-Cookie header multiple times for the same cookie

[gurgunday]

Hey everyone!

This PR closes #218 by decorating the reply with a Map to check if a cookie was already set during the lifecycle of a request. If that's the case, instead of multiplying the header, we now just update it.

Please let me know if there is an even better/more performant way to achieve the same outcome.

Finally, I got inspired (as usual) by @Uzlopak's suggestion.

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[Uzlopak]

I think we should discuss how we handle overwriting cookies. I think it is error prone to merge the attributes.

[gurgunday]

I think we should discuss how we handle overwriting cookies. I think it is error prone to merge the attributes.

Agreed, maybe a shallow comparison of the opts, or just domain and path

[gurgunday]

What do you think of this?

[Uzlopak]

Dont expect much from me. I am on vacation with my wife on crete and have limited access to my laptop. :))

Most stuff i currently do is with my phone. I will see if i can review it with my laptop.

[gurgunday]

Dont expect much from me. I am on vacation with my wife on crete and have limited access to my laptop. :))

Most stuff i currently do is with my phone. I will see if i can review it with my laptop.

It’s all good, have a great vacation 🏖️

[mcollina]

lgtm

I think this is worthwhile for a semver-major. Wdyt?

[Uzlopak]

I agree with @mcollina
The unit test "share options for setCookie and clearCookie" got changed, indicating that it is a semver major.

[gurgunday]

lgtm
I think this is worthwhile for a semver-major. Wdyt?

I agree, this is a behavior change at the API level as well.

[mcollina]

Can we avoid a change of the API? This module has quite a few dependencies and I fear it would be harder to update them all.

[gurgunday]

By API behavior change I meant this very specific scenario: user registers cookie, uses the decorators provided by cookie to set a cookie, then sets the Set-Cookie header manually with header('Set-Cookie', header)

In this case, the onSend hook will override the manually set header

We can revert it by simply appending to the header instead of setting it manually during onSend, will do that 👍

[mcollina]

Probably would be better, thx!

[mcollina]

Thanks!

[Uzlopak]

Is it the code coverage or is it the linter saying an else is mssing?

@gurgunday
You are absolutely right: Encapsulation could be a serious issue. Could be a massive perf eater, when different instances of fastify-cookie try to reconcile the cookies, as i proposed.
But still seams odd.

@mcollina
What do you think about collisions?

[gurgunday]

Is it the code coverage or is it the linter saying an else is mssing?

tap coverage, not the linter, adding istanbul ignore else fixed it

[Uzlopak]

My personal view on this PR:
Yes definitely an improvement and the collisiion issue could also happen in the previous implementation.
But would like have a final comment by @mcollina and/or @Eomm, if possible.

[mcollina]

lgtm

[mcollina]

I would still ship as semver-major but this looks very good now.

[gurgunday]

Latest results for 1 cookie

PR, OLD:

100 connections


┌─────────┬──────┬──────┬───────┬──────┬─────────┬─────────┬────────┐
│ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%  │ Avg     │ Stdev   │ Max    │
├─────────┼──────┼──────┼───────┼──────┼─────────┼─────────┼────────┤
│ Latency │ 1 ms │ 1 ms │ 2 ms  │ 2 ms │ 1.05 ms │ 0.76 ms │ 127 ms │
└─────────┴──────┴──────┴───────┴──────┴─────────┴─────────┴────────┘
┌───────────┬─────────┬─────────┬─────────┬─────────┬──────────┬─────────┬─────────┐
│ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg      │ Stdev   │ Min     │
├───────────┼─────────┼─────────┼─────────┼─────────┼──────────┼─────────┼─────────┤
│ Req/Sec   │ 69375   │ 69375   │ 75967   │ 76991   │ 75429.82 │ 2068.17 │ 69367   │
├───────────┼─────────┼─────────┼─────────┼─────────┼──────────┼─────────┼─────────┤
│ Bytes/Sec │ 15.1 MB │ 15.1 MB │ 16.5 MB │ 16.7 MB │ 16.4 MB  │ 447 kB  │ 15.1 MB │
└───────────┴─────────┴─────────┴─────────┴─────────┴──────────┴─────────┴─────────┘

Req/Bytes counts sampled once per second.
# of samples: 11

830k requests in 11.01s, 180 MB read

PR, NEW:

100 connections


┌─────────┬──────┬──────┬───────┬──────┬─────────┬─────────┬───────┐
│ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%  │ Avg     │ Stdev   │ Max   │
├─────────┼──────┼──────┼───────┼──────┼─────────┼─────────┼───────┤
│ Latency │ 1 ms │ 1 ms │ 2 ms  │ 2 ms │ 1.04 ms │ 0.59 ms │ 75 ms │
└─────────┴──────┴──────┴───────┴──────┴─────────┴─────────┴───────┘
┌───────────┬─────────┬─────────┬─────────┬─────────┬──────────┬─────────┬─────────┐
│ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg      │ Stdev   │ Min     │
├───────────┼─────────┼─────────┼─────────┼─────────┼──────────┼─────────┼─────────┤
│ Req/Sec   │ 73471   │ 73471   │ 81023   │ 81535   │ 79956.37 │ 2286.45 │ 73442   │
├───────────┼─────────┼─────────┼─────────┼─────────┼──────────┼─────────┼─────────┤
│ Bytes/Sec │ 15.9 MB │ 15.9 MB │ 17.6 MB │ 17.7 MB │ 17.3 MB  │ 495 kB  │ 15.9 MB │
└───────────┴─────────┴─────────┴─────────┴─────────┴──────────┴─────────┴─────────┘

Req/Bytes counts sampled once per second.
# of samples: 11

880k requests in 11.01s, 191 MB read

master:

100 connections


┌─────────┬──────┬──────┬───────┬──────┬─────────┬─────────┬───────┐
│ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%  │ Avg     │ Stdev   │ Max   │
├─────────┼──────┼──────┼───────┼──────┼─────────┼─────────┼───────┤
│ Latency │ 0 ms │ 1 ms │ 2 ms  │ 2 ms │ 1.03 ms │ 0.56 ms │ 79 ms │
└─────────┴──────┴──────┴───────┴──────┴─────────┴─────────┴───────┘
┌───────────┬─────────┬─────────┬───────┬─────────┬──────────┬────────┬─────────┐
│ Stat      │ 1%      │ 2.5%    │ 50%   │ 97.5%   │ Avg      │ Stdev  │ Min     │
├───────────┼─────────┼─────────┼───────┼─────────┼──────────┼────────┼─────────┤
│ Req/Sec   │ 76351   │ 76351   │ 83135 │ 83775   │ 82196.37 │ 2105   │ 76308   │
├───────────┼─────────┼─────────┼───────┼─────────┼──────────┼────────┼─────────┤
│ Bytes/Sec │ 16.6 MB │ 16.6 MB │ 18 MB │ 18.2 MB │ 17.8 MB  │ 457 kB │ 16.6 MB │
└───────────┴─────────┴─────────┴───────┴─────────┴──────────┴────────┴─────────┘

Req/Bytes counts sampled once per second.
# of samples: 11

904k requests in 11.01s, 196 MB read

[gurgunday]

The numbers budge, but for...of is consistently (slightly) faster on my machine

Here is why I think that is: .values creates an iterator object but calling next() on it is another method call and creates yet another object while for...of directly gets the value. Or maybe for...of causes an optimization since it's language syntax

Anyway I think we're good