-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
listen_socket: Support multiple options and allow socket options to be set before bind(). #2734
Changes from 17 commits
3109a2c
3495654
1b58b6f
a8fed75
824d8c7
fa23036
66b1ca3
7524e53
d569fec
a71b544
d8664f5
5650236
07792b7
33dda97
8cf748f
13887b8
4dc4b04
b154e73
14a9ec7
da28038
a46328b
d48f9b6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
#pragma once | ||
|
||
#include <memory> | ||
#include <vector> | ||
|
||
#include "envoy/common/pure.h" | ||
#include "envoy/network/address.h" | ||
|
@@ -30,36 +31,41 @@ class Socket { | |
*/ | ||
virtual void close() PURE; | ||
|
||
enum class SocketState { PreBind, PostBind }; | ||
|
||
/** | ||
* Visitor class for setting socket options. | ||
*/ | ||
class Options { | ||
class Option { | ||
public: | ||
virtual ~Options() {} | ||
virtual ~Option() {} | ||
|
||
/** | ||
* @param socket the socket on which to apply options. | ||
* @param state the current state of the socket. Significant for options that can only be | ||
* set for some particular state of the socket. | ||
* @return true if succeeded, false otherwise. | ||
*/ | ||
virtual bool setOptions(Socket& socket) const PURE; | ||
virtual bool setOption(Socket& socket, SocketState state) const PURE; | ||
|
||
/** | ||
* @return bits that can be used to separate connections based on the options. Should return | ||
* zero if connections with different options can be pooled together. This is limited | ||
* to 32 bits to allow these bits to be efficiently combined into a larger hash key | ||
* used in connection pool lookups. | ||
* @param vector of bits that can be used to separate connections based on the options. Should | ||
* return zero if connections with different options can be pooled together. This is | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no return now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment updated, thanks for noticing this! |
||
* limited to 32 bits to allow these bits to be efficiently combined into a larger hash | ||
* key used in connection pool lookups. | ||
*/ | ||
virtual uint32_t hashKey() const PURE; | ||
virtual void hashKey(std::vector<uint8_t>& key) const PURE; | ||
}; | ||
typedef std::shared_ptr<Options> OptionsSharedPtr; | ||
typedef std::unique_ptr<Option> OptionPtr; | ||
typedef std::shared_ptr<std::vector<OptionPtr>> OptionsSharedPtr; | ||
|
||
/** | ||
* Set the socket options for later retrieval with options(). | ||
* Add a socket option visitor for later retrieval with options(). | ||
*/ | ||
virtual void setOptions(const OptionsSharedPtr&) PURE; | ||
virtual void addOption(OptionPtr&&) PURE; | ||
|
||
/** | ||
* @return the socket options stored earlier with setOptions(), if any. | ||
* @return the socket options stored earlier with addOption() calls, if any. | ||
*/ | ||
virtual const OptionsSharedPtr& options() const PURE; | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,9 +4,9 @@ | |
#include <cstdint> | ||
#include <functional> | ||
#include <list> | ||
#include <map> | ||
#include <memory> | ||
#include <string> | ||
#include <unordered_map> | ||
#include <vector> | ||
|
||
#include "envoy/config/bootstrap/v2/bootstrap.pb.h" | ||
|
@@ -199,17 +199,7 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable<Logger::Id::u | |
*/ | ||
struct ThreadLocalClusterManagerImpl : public ThreadLocal::ThreadLocalObject { | ||
struct ConnPoolsContainer { | ||
typedef std::unordered_map<uint64_t, Http::ConnectionPool::InstancePtr> ConnPools; | ||
|
||
uint64_t key(ResourcePriority priority, Http::Protocol protocol, uint32_t hash_key) { | ||
// One bit needed for priority | ||
static_assert(NumResourcePriorities == 2, | ||
"Fix shifts below to match number of bits needed for 'priority'"); | ||
// Two bits needed for protocol | ||
static_assert(Http::NumProtocols <= 4, | ||
"Fix shifts below to match number of bits needed for 'protocol'"); | ||
return uint64_t(hash_key) << 3 | uint64_t(protocol) << 1 | uint64_t(priority); | ||
} | ||
typedef std::map<std::vector<uint8_t>, Http::ConnectionPool::InstancePtr> ConnPools; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why switch from unordered to ordered? Is it just because you don't have a hash function? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right. std::map supports a vector as a key out of the box, for unordered map I'd need to provide the hash function, I guess. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looked into possible hash support, boost::hash would be nice, as there is no std hash combiner. Or then just keep this as unordered map for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I've run into this before too. I'm fine leaving it as std::map, although other reviewers may feel more strongly about it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No strong opinion that this has to be unordered, unless it affect performance. std::hash does support string as key so if we need a hash function we can just reuse some from that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, no strong preference. IMHO, the STL has a lot of inconsistency around how hashing and comparison works with containers and inbuilt types, let's fix it only when we need to for perf reasons and prefer simplicity otherwise. |
||
|
||
ConnPools pools_; | ||
uint64_t drains_remaining_{}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this is now a singular option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.