pistacheio / pistache

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

Plans to release Pistache 0.1 #493

Open knowledge4igor opened 5 years ago

knowledge4igor commented 5 years ago

Hello,

Now I think it's a good time to talk about planes to Pistache 0.1 release.

1) Do we have serious bugs (blockers) that stop 0.1 release? If they are, then can we write down them?

2) I have run unit tests several times and I did not see any flaky tests. Maybe you have different results.

3) Also I have run memcheck several times:

$ cmake --build . --target test_memcheck

several times and have the following results:

...
100% tests passed, 0 tests failed out of 23

Total Test time (real) = 355.91 sec

Don't see any defects. Maybe on your hardware and Linux OS you have different results, let's discuss it too.

Now I propose to stop (until release) to accept PR (I mean merge them to master) with new features: only bug fixes, code improvements and unit tests for features that have already been implemented.

What do you think about it? Any ideas?

arthurafarias commented 5 years ago

Now we can add memory check as a required CI component.

dennisjenkins75 commented 5 years ago

Did we fix the denial of service bug where someone can hold a socket open and not flush the full HTTP header and essentially consume a pistache thread?

Does the pistache "echo" test crash when under sustained load from apache "ab"?

We should enable memory leak checking on the presubmit tests. And possibly "tsan".

Its not a bug in pistache, but the travis-ci framework is very frustrating. The tests often fail due to the test system itself not coming up (failure to fetch packages).

I need to check if github makes it easy to "tag" bugs or group them. sadly, I am very busy right now and don't have a lot of time to do much other than rubber-stamp PRs. :(

dennisjenkins75 commented 5 years ago

We might also want to tackle issue #471, agree upon a code-formatting style (I'm flexible, so long as the indenting tool can be ran from the Linux CLI and the config is checked into github), then reformat the code (after final bugs are fixed, before cutting the release).

knowledge4igor commented 5 years ago

@dennisjenkins75

Did we fix the denial of service bug where someone can hold a socket open and not flush the full HTTP header and essentially consume a pistache thread?

and

Does the pistache "echo" test crash when under sustained load from apache "ab"?

Sounds like serious bugs. Can you tell me link to issue or describe steps to reproduce these problems? I will try to find time to dive into them.

arthurafarias commented 5 years ago

I made some progress in this problem some time ago.

The problem I detected was: independently of having or not keep-alive header, the socket remained open waiting for a "keep-alive" behaviour from client.

As MDN says in https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection

The Connection general header controls whether or not the network connection stays open after the current transaction finishes. If the value sent is keep-alive, the connection is persistent and not closed, allowing for subsequent requests to the same server to be done.

My approach was. If there is a connection header with keep-alive, the socket won't be closed after the response. If there is nothing or connection header with an explicit close. the server will close the connection after sending a response. This was done in pull request #462.

The steps to reproduce the problem are in issue #333.

knowledge4igor commented 5 years ago

@dennisjenkins75 Hello,

Does the pistache "echo" test crash when under sustained load from apache "ab"?

I have started http_server from example (with default options) and tried to run ab with different options, for instance:

$ ab -c 200 -n 100000 -k http://127.0.0.1:9080/
This is ApacheBench, Version 2.3 <$Revision: 1807734 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking 127.0.0.1 (be patient)
Completed 10000 requests
Completed 20000 requests
Completed 30000 requests
Completed 40000 requests
Completed 50000 requests
Completed 60000 requests
Completed 70000 requests
Completed 80000 requests
Completed 90000 requests
Completed 100000 requests
Finished 100000 requests

Server Software:        
Server Hostname:        127.0.0.1
Server Port:            9080

Document Path:          /
Document Length:        0 bytes

Concurrency Level:      200
Time taken for tests:   20.104 seconds
Complete requests:      100000
Failed requests:        0
Non-2xx responses:      100000
Keep-Alive requests:    100000
Total transferred:      6900000 bytes
HTML transferred:       0 bytes
Requests per second:    4974.10 [#/sec] (mean)
Time per request:       40.208 [ms] (mean)
Time per request:       0.201 [ms] (mean, across all concurrent requests)
Transfer rate:          335.17 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.4      0      14
Processing:    17   40  15.5     35     156
Waiting:       17   40  15.5     35     156
Total:         17   40  15.5     35     156

Percentage of the requests served within a certain time (ms)
  50%     35
  66%     42
  75%     45
  80%     48
  90%     61
  95%     72
  98%     86
  99%     94
 100%    156 (longest request)

and didn't get any crashes. Please, can you tell me some steps of reproducing problem with ab load testing?.

dennisjenkins75 commented 5 years ago

I repeated your ab test, and ran several variants of it (including using many more threads in the http_server and multiple concurrent 'ab' running). Result: no crashes. Yeah!

Issue #237 discusses the denial of service attack of just opening a TCP socked and never sending the http header. I can reproduce this issue, but am unsure if we want to hold up a 0.1 release to address it or not.

arthurafarias commented 5 years ago

We can't release with a known bug. Maybe we should tag this release as pre-alpha. I really recommend using a semantic versioning strategy.

hydratim commented 5 years ago

I'd like to see us mitigate the slow-loris attack vector before the 0.1 release - I'll see if I can take a look at it this weekend. It's a fairly common issue with webservers, so there has been lots of prior research on this subject , so fixing it shouldn't be too difficult.

arthurafarias commented 5 years ago

@knowledge4igor, do you know if there is any PEG header only library in C++ viable to use in this project? There is a problem in client address that is:

1 - If we use a URL different than an ipv4, ipv6 or localhost, the client will crash. #420.

Yesterday I was trying to figure out if there was any simple way of parsing URLs in C++. The regex strategy is really complex.

I was trying to replace the way that this library parses addresses.

I wrote a pseudocode to parse URLs and I was trying to find something stable to use with C++ to parse with PEGs:

url <- ^({{protocol}}{{auth}}{{host}}{{port}}{{path}}{{query}}{{hash}})$
protocol <- (?:(http|https)\:\/\/|)
host <- ({{domain}}|{{ipv4}}|\[{{ipv6}}\])
port <- (?:\:([0-9]{1,5})|)
path <- (\/(?:{{urlencoded}}\/?)+|)
auth <- (?:{{user}}(?:\:?{{password}}?)\@|)
user <- ({{urlencoded}})
password <- ({{urlencoded}})
query <- (\?(?:{{urlencoded}}+|{{urlencoded}}+\=|{{urlencoded}}+\=|{{urlencoded}}+\={{urlencoded}}+|{{urlencoded}}+\={{urlencoded}}+)(?:&|&{{urlencoded}}+|&{{urlencoded}}+\=|&{{urlencoded}}+\=|&{{urlencoded}}+\={{urlencoded}}+)+)
hash <- (\#?(?:.*)|)
urlencoded <- (?:(?:%[0-9A-F]{2}|[a-zA-Z0-9])*)
domain <- (?:(?:[a-zA-Z0-9]+(?:\.[a-zA-Z0-9]+)?)|(?:(?:[a-zA-Z0-9]+[\-\_]*[a-zA-Z0-9]+(?:\.[a-zA-Z0-9]+)?)|))
ipv4 <- (?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))
ipv6 <- (?:(?:(?:(?:(?:(?:(?:[0-9a-fA-F]{1,4})):){6})(?:(?:(?:(?:(?:[0-9a-fA-F]{1,4})):(?:(?:[0-9a-fA-F]{1,4})))|(?:(?:(?:(?:(?:25[0-5]|(?:[1-9]|1[0-9]|2[0-4])?[0-9]))\.){3}(?:(?:25[0-5]|(?:[1-9]|1[0-9]|2[0-4])?[0-9])))))))|(?:(?:::(?:(?:(?:[0-9a-fA-F]{1,4})):){5})(?:(?:(?:(?:(?:[0-9a-fA-F]{1,4})):(?:(?:[0-9a-fA-F]{1,4})))|(?:(?:(?:(?:(?:25[0-5]|(?:[1-9]|1[0-9]|2[0-4])?[0-9]))\.){3}(?:(?:25[0-5]|(?:[1-9]|1[0-9]|2[0-4])?[0-9])))))))|(?:(?:(?:(?:(?:[0-9a-fA-F]{1,4})))?::(?:(?:(?:[0-9a-fA-F]{1,4})):){4})(?:(?:(?:(?:(?:[0-9a-fA-F]{1,4})):(?:(?:[0-9a-fA-F]{1,4})))|(?:(?:(?:(?:(?:25[0-5]|(?:[1-9]|1[0-9]|2[0-4])?[0-9]))\.){3}(?:(?:25[0-5]|(?:[1-9]|1[0-9]|2[0-4])?[0-9])))))))|(?:(?:(?:(?:(?:(?:[0-9a-fA-F]{1,4})):){0,1}(?:(?:[0-9a-fA-F]{1,4})))?::(?:(?:(?:[0-9a-fA-F]{1,4})):){3})(?:(?:(?:(?:(?:[0-9a-fA-F]{1,4})):(?:(?:[0-9a-fA-F]{1,4})))|(?:(?:(?:(?:(?:25[0-5]|(?:[1-9]|1[0-9]|2[0-4])?[0-9]))\.){3}(?:(?:25[0-5]|(?:[1-9]|1[0-9]|2[0-4])?[0-9])))))))|(?:(?:(?:(?:(?:(?:[0-9a-fA-F]{1,4})):){0,2}(?:(?:[0-9a-fA-F]{1,4})))?::(?:(?:(?:[0-9a-fA-F]{1,4})):){2})(?:(?:(?:(?:(?:[0-9a-fA-F]{1,4})):(?:(?:[0-9a-fA-F]{1,4})))|(?:(?:(?:(?:(?:25[0-5]|(?:[1-9]|1[0-9]|2[0-4])?[0-9]))\.){3}(?:(?:25[0-5]|(?:[1-9]|1[0-9]|2[0-4])?[0-9])))))))|(?:(?:(?:(?:(?:(?:[0-9a-fA-F]{1,4})):){0,3}(?:(?:[0-9a-fA-F]{1,4})))?::(?:(?:[0-9a-fA-F]{1,4})):)(?:(?:(?:(?:(?:[0-9a-fA-F]{1,4})):(?:(?:[0-9a-fA-F]{1,4})))|(?:(?:(?:(?:(?:25[0-5]|(?:[1-9]|1[0-9]|2[0-4])?[0-9]))\.){3}(?:(?:25[0-5]|(?:[1-9]|1[0-9]|2[0-4])?[0-9])))))))|(?:(?:(?:(?:(?:(?:[0-9a-fA-F]{1,4})):){0,4}(?:(?:[0-9a-fA-F]{1,4})))?::)(?:(?:(?:(?:(?:[0-9a-fA-F]{1,4})):(?:(?:[0-9a-fA-F]{1,4})))|(?:(?:(?:(?:(?:25[0-5]|(?:[1-9]|1[0-9]|2[0-4])?[0-9]))\.){3}(?:(?:25[0-5]|(?:[1-9]|1[0-9]|2[0-4])?[0-9])))))))|(?:(?:(?:(?:(?:(?:[0-9a-fA-F]{1,4})):){0,5}(?:(?:[0-9a-fA-F]{1,4})))?::)(?:(?:[0-9a-fA-F]{1,4})))|(?:(?:(?:(?:(?:(?:[0-9a-fA-F]{1,4})):){0,6}(?:(?:[0-9a-fA-F]{1,4})))?::))))

How this library is parsing http path and query?

hydratim commented 5 years ago

Shouldn't we break the client out into it's own project; It's really odd to be maintaining it in the same project as the server library and it's making maintaining the network backend overly complex.

The Client and Server have really different requirements for backend networking.

knowledge4igor commented 5 years ago

@dennisjenkins75 I will think about #237 issue at the weekend

knowledge4igor commented 5 years ago

We can't release with a known bug. Maybe we should tag this release as pre-alpha. I really recommend using a semantic versioning strategy.

I think that now we are ready to 0.1 alpha version.

hydratim commented 5 years ago

237 is still open - I haven't found the time to look at it yet

mohsenomidi commented 4 years ago

Hi, I just see this open bugs now...Can you find the solution for issue #237? If you have a solution and don’t have time to development, i can do it in my free times... Meanwhile i will take a look to this issue.

kiplingw commented 4 years ago

I should add that versioning as it actually is in the source is a 3-tuple. e.g. I think what we're talking about is releasing version 0.1.0. To bump the version edit version.txt and then dch -i from the source root. I do this which each new unstable release pushed to the PPA.

mohsenomidi commented 4 years ago

@dennisjenkins75

I check the bug no: #237 and find the root cause of this bug and will simply fixed,but I need investigate to get the event poll structure, it has complicated structure, and need some assistance. This thread is good for asking question and made code changes or need to open new issue or PR ?

mohsenomidi commented 4 years ago

@kiplingw Hi kip, i think you can help me to fix this issue if you are familiar with Pistache::Epoll and Pistache::Event structure, I add some code to check connections status during requests receive, now i can detect the silence connection as the timeout(the root cause has been solved), after this we should erase the registered event from the Pistache:Epoll vector, in this part i am stuck... The design structure is complicated and need assistance..

kiplingw commented 4 years ago

Hey @mohsenomidi. Sorry, but I'm sadly not the right guy for this. I could probably help if I had time, but right now it's very limited.

mohsenomidi commented 4 years ago

Hi @kiplingw , thanks :)

by the way, I investigate the source and find some info, i will share it here and can be useful to bugfix.

in transport.cc, Pistache had the Transport::onReady function to get the List of fds to process it, in main for the connection detect as peer and handlePeerQueuewill called, this function call the handlePeer, at the end of function this FD register with two event notification 1-read , 2-shutdown, this is the problem there is no exclusive event to detecting the timeout for the connected client. then event in the Transport::onReady never get isReadableand there is no condition for timeout, there for the fd stay remaining open...

Now the specific notify should be register as timeout and in in handlePeer should be have something like this : eactor()->registerFd(key(), fd, NotifyOn::Read | NotifyOn::Shutdown | NotifyOn::Timeout, Polling::Mode::Edge); then epoll can triggered by getting timeout.

and should be have the condition like this for entry.onTimeout() in Transport::onReady(const Aio::FdSet& fds) to close the FD or handlePeerDisconnection

kiplingw commented 4 years ago

Happy to learn you've gotten further in your discovery. When you're ready for a PR, feel free to submit it.

Tachi107 commented 2 years ago

@kiplingw, @dennisjenkins75, I was thinking about tagging the current master branch as Pistache 0.0.5, so that I can finally start on packaging it for Debian. It this fine for you?

kiplingw commented 2 years ago

@kiplingw, @dennisjenkins75, I was thinking about tagging the current master branch as Pistache 0.0.5, so that I can finally start on packaging it for Debian. It this fine for you?

Yup. Good idea.

Tachi107 commented 2 years ago

The Debian package is ready, I've uploaded it here: https://salsa.debian.org/tachi/pistache

As I'm not yet a Debian Developer I have to wait for somebody to upload it to the Debian archive for me.

kiplingw commented 2 years ago

Thanks @Tachi107. I've let @pabs3 know. I think we're still awaiting a Debian maintainer.

pabs3 commented 2 years ago

@Tachi107 you will need to read the intro guide, in particular the request for sponsor (RFS) step is currently missing.

pabs3 commented 2 years ago

PS: please note that even if you don't get a sponsor immediately, keep updating the package on mentors.d.n and keep updating the RFS bug when the package version changes.

Tachi107 commented 2 years ago

@pabs3 thanks for reminding me of the RFS. By the way, I've been uploading packages for a while now, I'm a DM :D

pabs3 commented 2 years ago

Ah, woops! I thought your name sounded familiar :)

-- bye, pabs

https://bonedaddy.net/pabs3/

kiplingw commented 2 years ago

@pabs3 thanks for reminding me of the RFS. By the way, I've been uploading packages for a while now, I'm a DM :D

YES!!!