22 implement connection class

[gwolf-011235]

Summary after Update

Testing of Server functions was rather difficult since many clib functions. Abstracted away to make mocking possible.

1. Modified Class Connection made into Struct Connection
2. New Class EpollWrapper
3. New Class SocketPolicy
4. Redesign of server functions
5. Redesign of handleConnection
6. Added tests

1. Struct Connection

• Connection holds various data about a active connection
• But it doesn't really act itelf, it just holds data, therefore made into struct
• Also moved connection buffers from server into a buffer of struct Connection

2. Class EpollWrapper

• Epoll is now abstracted away in own class
• Opens epoll instance and handles events
• Gets passed to Server upon construction, can also be mocked

3. Class SocketPolicy

• Abstracts away various Socket related C Functions: accept(), listen(), recv() etc
• Gets passed to Server upon construction, can also be mocked

4. Redesign of server functions

• Many functions of Server are pulled out of the class to make them easily testable: They get passed a Server instance, which can be prepared as needed and inspected after function
• Server was equipped with Getters and Setters as well as Wrapper functions to make testing possible

5. Redesign of handleConnection

• handleConnection is now split in different stages, depending on ConnectionStatus
• Makes it possible to server several connections by working on only a part of a specific connection
• Up for debate

6. Tests

• Much of the rework was done to make testing easier
• Since every function is isolated testable, it's very easy to test.

Summary before Update

1. New Class Connection
2. New Struct Socket
3. Separation of epoll and socket opening: Server can now open one or more Sockets as Virtual Servers to listen on
4. Handling of events either on VirtualServerSockets or Connections
5. Handle Timeout
6. Various

1. Connection

• The class Connection() represents one active connection and saves corresponding information
• It stores info about the connected sockets of server and client.
• Saves and updates time since last event
• Has an enum for different states of a connection (not implemented yet).
• Saves number of received bytes

2. Socket

• Simple data struct of fd, host string and port string

3. Separation of epoll and socket opening

• Server now has a std::map of Sockets representing all open Virtual Servers
• Constructor opens epoll instance.
• Then runs initVirtualServers(): try to open all sockets defined in the ConfigFile. Duplicate sockets are ignored (two virtual servers can run on the same socket, but have a different name).
• For translating IP and port to number getaddrinfo() is used.
• FD, host and port are saved in Socket struct which are then saved in map

4. Handling of events

• After epoll returns the list of FDs, checks if the fd is in the map m_virtualServers.
• If yes tries to accept connection.
• When a new connection is accepted, a new Connection() is saved in the map m_connections
• If no it handles the connection
• The event mask, returned by epoll is used in acceptConnections: if it is EPOLLERR close server. If it is not EPOLLIN report error (this should not happen). Else do work
• Event mask not yet implemented in handleConnections()

5. Handle Timeout

• Connection class saves time elapsed since last action
• After Server is finished handling all reported events it loops through active connections and checks for timeout.
• If timeout removes from epoll and closes the connection.

6. Various

• Chose a simpler implementation instead of virtual dispatch
• Testing seems hard, since many library functions used - could add to Mock class or make new one
• Enum of Connection() could represents different states of the connection like parsing request, body, building response, sending response.
• Last state would be needed if not full response could be sent via the first send(), so we would come back later
• Added some static ints (epollTimeout, backlog etc) to configure server. Could be passed in constructor or maybe defined in ConfigFile

[gwolf-011235]

Regarding the tests

Yes I can create a class which wraps the free functions and mock this wrapper class. This class would be passed to the server (similar to FileSystemPolicy()) so it can be injected as mock class also. Would you rather use the existing class (and maybe rename it to LibCWrapper() or something) or create a new one?

Also I would add this as an additional PR. Or should I expand on this one?

[u413-284-si]

Regarding the tests

Yes I can create a class which wraps the free functions and mock this wrapper class. This class would be passed to the server (similar to FileSystemPolicy()) so it can be injected as mock class also. Would you rather use the existing class (and maybe rename it to LibCWrapper() or something) or create a new one?

Also I would add this as an additional PR. Or should I expand on this one?

I would create a new one specific to these functions and expand this PR as it is part of the testing procedure imo.

[gwolf-011235]

I've got a problem with testing!

In principle it is said that you should not test private functions because you should test the interface and not the implementation. This improves encapsulation / abstraction and maintains flexibility in the implementation. Sounds good - in principle.

In this context, I also read about "iceberg" classes. These are classes that combine (too) much functionality and then only have one or two public functions ("tip of the iceberg"). This should be avoided, since it violates the Single Responsibility Principle and a bunch of other principles. It also makes testing complicated, leading to poorer coverage. The advice is to create smaller, focused classes with clear responsibilities and well-defined public interfaces. I tried to go with this approach with the ResponseBuilder: of course you suddenly have a lot of classes.

Or you can ignore the recommendation and penetrate the class and test the private functions as a friend or with preprocessor fun (private = public).

There is also the approach of not putting it in any class, but use free functions that can be easily tested in isolation. This is often found in data-oriented programming. But also object oriented style points into that direction: for example Scott Meyers says non-member functions improve encapsulation: https://www.aristeia.com/Papers/CUJ_Feb_2000.pdf (written way back in 2000!) I chose this approach for #22. The disadvantage here is that the function params explode quickly.

There are currently two dependencies to mock in the server:

• EpollWrapper (simulates an epoll instance) and
• SocketPolicy (has functions such as socket() and accept() ). Both are injected so that the mock variant can be used if required.

The server itself is set up in such a way that it initializes the virtual server(s) in the constructor and then waits for connections in an endless loop. I find testing difficult because I can't check the status of the server from the outside. For example if I init a dummy server, I would need to access the m_vitualServer map inside the server, to check if everything set up correctly.

With the free functions it's a bit silly because you have to enter so many params. But it is extremely easy to test, since you can check each function in isolation.

Any different ideas? I have to admit I like the free function approach, but the param list can be daunting.

[QCHR1581]

I've got a problem with testing!

In principle it is said that you should not test private functions because you should test the interface and not the implementation. This improves encapsulation / abstraction and maintains flexibility in the implementation. Sounds good - in principle.

In this context, I also read about "iceberg" classes. These are classes that combine (too) much functionality and then only have one or two public functions ("tip of the iceberg"). This should be avoided, since it violates the Single Responsibility Principle and a bunch of other principles. It also makes testing complicated, leading to poorer coverage. The advice is to create smaller, focused classes with clear responsibilities and well-defined public interfaces. I tried to go with this approach with the ResponseBuilder: of course you suddenly have a lot of classes.

Or you can ignore the recommendation and penetrate the class and test the private functions as a friend or with preprocessor fun (private = public).

There is also the approach of not putting it in any class, but use free functions that can be easily tested in isolation. This is often found in data-oriented programming. But also object oriented style points into that direction: for example Scott Meyers says non-member functions improve encapsulation: https://www.aristeia.com/Papers/CUJ_Feb_2000.pdf (written way back in 2000!) I chose this approach for #22. The disadvantage here is that the function params explode quickly.

There are currently two dependencies to mock in the server:

• EpollWrapper (simulates an epoll instance) and
• SocketPolicy (has functions such as socket() and accept() ). Both are injected so that the mock variant can be used if required.

The server itself is set up in such a way that it initializes the virtual server(s) in the constructor and then waits for connections in an endless loop. I find testing difficult because I can't check the status of the server from the outside. For example if I init a dummy server, I would need to access the m_vitualServer map inside the server, to check if everything set up correctly.

With the free functions it's a bit silly because you have to enter so many params. But it is extremely easy to test, since you can check each function in isolation.

Any different ideas? I have to admit I like the free function approach, but the param list can be daunting.

I am not quite sure if I understand that correctly, but why can't you access the m_virtualServers map for testing purposes?

[gwolf-011235]

I would need to write a getter for the m_virtualServers map, since it's private.

The sole purpose (right now) of this getter would be for testing.

I could also have getters/setters to reduce the free function params, and pass a ref to server (I think this would be the way of the article)

Right now it does work. The question is: is there a better alternative?

[QCHR1581]

I would need to write a getter for the m_virtualServers map, since it's private.

The sole purpose (right now) of this getter would be for testing.

I could also have getters/setters to reduce the free function params, and pass a ref to server (I think this would be the way of the article)

Right now it does work. The question is: is there a better alternative?

I didn't mention it before, but the article was really an interesting read. I never thought about the use of non-friend non-member functions instead of member functions for the purpose of increasing the encapsulation. Thanks for for the link!

In terms of your need to check the server status, I am not absolutely sure what the best approach is. GoogleTest provides a FRIEND_TEST macro for enabling the test to access private class members. Perhaps this is an alternative solution?

At the end of the project we have to remove all test we have created when we submit the project. Therefore you could use this test and then deleted when it is not needed anymore.

[gwolf-011235]

I think friend testing has the problem that you have to add (and in our case remove) friends to the class. I think this is too intrusive especially since we have full control over our code.

Right now I lean towards adding getters and other interface changes to make it more testable.

Then I'll still have non member functions which get passed a reference to server. That way the amount of things to pass to the functions is reduced.

I'm not sure if it's good design then, but at least it makes it testable (which kinda also in itself is good design)

[u413-284-si]

To weigh in this discussion I can share my current experience at work:

We are creating Unit tests for existing classes to increase the test coverage of our code base. The approach to many of those classes is to add additional methods (getters/setters/helper functions) as well as Constructors which serve only for testing - and are marked as such. I cannot say if that's good practice, but it is at least not uncommon.

Therefore I am as well in favour of extending the interface to enable better testing. External functions with lots of parameters should best be kept to a minimum.

[gwolf-011235]

Good update for making the server easier testable, using struct instead of class for Connection and also abstracting functions!

One thing I want to emphasize about this PR is that it divides the processing of a connection into different phases (ReceiveRequest, ReceiveBody etc)

I think this way we can serve multiple clients better since we don't handle everything from one connection and then work on the next connection. We do a little bit here and a little bit there. This gives us flexibility:

• If a client sends a big file we don't read until after he is finished. We read a bit and then come back later.
• If a client wants to send garbage we can close the connection earlier by just looking at the request. If it is malformed we close and don't read until the client is finished.
• If we send a response and can only send half, we can come back later and try again.

We could also use a process per connection, since fork() is allowed. But I think that makes everything harder to deal with. We will have to use fork for CGI anyway.