pistacheio / pistache

A high-performance REST toolkit written in C++
https://pistacheio.github.io/pistache/
Apache License 2.0
3.12k stars 690 forks source link

add feature keepalive #1095

Closed zydxhs closed 1 year ago

zydxhs commented 1 year ago

This is a follow up to #1061, add feature keepalive, and fix the issue #1030.

kiplingw commented 1 year ago

Thanks @zydxhs. It looks like you've got some CI issues to work through. When you're ready, just holler.

zydxhs commented 1 year ago

@kiplingw I fixed this 'conventional-commits / commitlint (pull_request)', but others I don't why failed.

I run the tests successed.

Tachi107 commented 1 year ago

It seems that the failures are related to the changes made with this PR. They seem to point to a memory management error.

Tachi107 commented 1 year ago

I'm really happy to see how effective it is to run the same tests under different platforms on different compiler versions and different sanitizers :D

For example, look how the same compiler version on the same OS family behaves differently on two different OS releases; both RHEL 8 and RHEL 9 ship LLVM 13, but Clang only detects issues on RHEL 9. Fascinating.

Less happy about the fact that this PR has issues, but they can be fixed :)

kiplingw commented 1 year ago

Nice job @Tachi107. Thanks to your efforts at ensuring our CI infrastructure works properly, we can now all see that it does! Obviously this isn't a solution to the failures, but at least now we know that there may be problems with his PR.

@zydxhs, you might want to play with Valgrind locally. It's a very helpful tool. I suggest starting by trying to reproduce the problem.

zydxhs commented 1 year ago

@kiplingw @Tachi107 I can't reproduce it.

It is possible that Http::Header::Collection does not define the movement constructor and the movement assignment function caused the problem, but I couldn't verify it. so can you verify it?

image

Tachi107 commented 1 year ago

I can't reproduce it.

What compiler and flags did you use?

zydxhs commented 1 year ago

@Tachi107 I got it. I'm not add this param to meson -Db_coverage=true -Db_lundef=false -Db_sanitize=address

codecov-commenter commented 1 year ago

Codecov Report

Merging #1095 (9809038) into master (dc04fdc) will decrease coverage by 0.00%. The diff coverage is 96.72%.

@@            Coverage Diff             @@
##           master    #1095      +/-   ##
==========================================
- Coverage   78.32%   78.32%   -0.01%     
==========================================
  Files          53       53              
  Lines        6657     6707      +50     
==========================================
+ Hits         5214     5253      +39     
- Misses       1443     1454      +11     
Impacted Files Coverage Δ
include/pistache/listener.h 0.00% <ø> (ø)
include/pistache/transport.h 53.75% <ø> (-6.25%) :arrow_down:
src/server/endpoint.cc 83.33% <93.75%> (+1.71%) :arrow_up:
include/pistache/endpoint.h 95.65% <100.00%> (+0.65%) :arrow_up:
src/common/http.cc 74.65% <100.00%> (+0.19%) :arrow_up:
src/common/peer.cc 77.02% <100.00%> (-0.44%) :arrow_down:
src/common/transport.cc 67.94% <100.00%> (-0.70%) :arrow_down:
src/server/listener.cc 69.87% <100.00%> (+1.31%) :arrow_up:
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

kiplingw commented 1 year ago

LGTM. @Tachi107?

Tachi107 commented 1 year ago

I'll review the changes later today, but as CI is passing I believe this is mergeable

kiplingw commented 1 year ago

Ok, feel free to merge after you've had a look through his changes. I'm fine with them.

kiplingw commented 1 year ago

Good eye @Tachi107.

zydxhs commented 1 year ago

@Tachi107 I have updated it according to your suggestion.

kiplingw commented 1 year ago

Thanks @zydxhs!

Tachi107 commented 1 year ago

Thanks for the patch, @zydxhs :)