Avoid returning references that are mutex protected (#297)
* Fix unsafe calls and safeguard WebSocketMessage from being called w/ temporaries * Use unnamed namespace to express internal linkage * Avoid returning references that are mutex protected Motivation for this MR The antipattern of returning references to mutex protected members was removed. Since a caller can hold the reference it would make all class level locking meaningless. Instead values are returned. The IXWebSocketPerMessageDeflateOptions class was shrunk by 7 bytes (1 padding + 2*3) after changing the int members to the used uint8_t; side effects of that were handled. An inefficient "string -> int" was replaced by standard library. As seen here http://coliru.stacked-crooked.com/a/46b5990bafb9c626 this gives an order of magnitude better performance.
This commit is contained in:
		@@ -41,7 +41,8 @@ namespace ix
 | 
				
			|||||||
        , _pingIntervalSecs(kDefaultPingIntervalSecs)
 | 
					        , _pingIntervalSecs(kDefaultPingIntervalSecs)
 | 
				
			||||||
    {
 | 
					    {
 | 
				
			||||||
        _ws.setOnCloseCallback(
 | 
					        _ws.setOnCloseCallback(
 | 
				
			||||||
            [this](uint16_t code, const std::string& reason, size_t wireSize, bool remote) {
 | 
					            [this](uint16_t code, const std::string& reason, size_t wireSize, bool remote)
 | 
				
			||||||
 | 
					            {
 | 
				
			||||||
                _onMessageCallback(
 | 
					                _onMessageCallback(
 | 
				
			||||||
                    ix::make_unique<WebSocketMessage>(WebSocketMessageType::Close,
 | 
					                    ix::make_unique<WebSocketMessage>(WebSocketMessageType::Close,
 | 
				
			||||||
                                                      emptyMsg,
 | 
					                                                      emptyMsg,
 | 
				
			||||||
@@ -75,7 +76,7 @@ namespace ix
 | 
				
			|||||||
        _extraHeaders = headers;
 | 
					        _extraHeaders = headers;
 | 
				
			||||||
    }
 | 
					    }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    const std::string& WebSocket::getUrl() const
 | 
					    const std::string WebSocket::getUrl() const
 | 
				
			||||||
    {
 | 
					    {
 | 
				
			||||||
        std::lock_guard<std::mutex> lock(_configMutex);
 | 
					        std::lock_guard<std::mutex> lock(_configMutex);
 | 
				
			||||||
        return _url;
 | 
					        return _url;
 | 
				
			||||||
@@ -94,7 +95,7 @@ namespace ix
 | 
				
			|||||||
        _socketTLSOptions = socketTLSOptions;
 | 
					        _socketTLSOptions = socketTLSOptions;
 | 
				
			||||||
    }
 | 
					    }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    const WebSocketPerMessageDeflateOptions& WebSocket::getPerMessageDeflateOptions() const
 | 
					    const WebSocketPerMessageDeflateOptions WebSocket::getPerMessageDeflateOptions() const
 | 
				
			||||||
    {
 | 
					    {
 | 
				
			||||||
        std::lock_guard<std::mutex> lock(_configMutex);
 | 
					        std::lock_guard<std::mutex> lock(_configMutex);
 | 
				
			||||||
        return _perMessageDeflateOptions;
 | 
					        return _perMessageDeflateOptions;
 | 
				
			||||||
@@ -384,7 +385,8 @@ namespace ix
 | 
				
			|||||||
                [this](const std::string& msg,
 | 
					                [this](const std::string& msg,
 | 
				
			||||||
                       size_t wireSize,
 | 
					                       size_t wireSize,
 | 
				
			||||||
                       bool decompressionError,
 | 
					                       bool decompressionError,
 | 
				
			||||||
                       WebSocketTransport::MessageKind messageKind) {
 | 
					                       WebSocketTransport::MessageKind messageKind)
 | 
				
			||||||
 | 
					                {
 | 
				
			||||||
                    WebSocketMessageType webSocketMessageType;
 | 
					                    WebSocketMessageType webSocketMessageType;
 | 
				
			||||||
                    switch (messageKind)
 | 
					                    switch (messageKind)
 | 
				
			||||||
                    {
 | 
					                    {
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -92,8 +92,8 @@ namespace ix
 | 
				
			|||||||
        ReadyState getReadyState() const;
 | 
					        ReadyState getReadyState() const;
 | 
				
			||||||
        static std::string readyStateToString(ReadyState readyState);
 | 
					        static std::string readyStateToString(ReadyState readyState);
 | 
				
			||||||
 | 
					
 | 
				
			||||||
        const std::string& getUrl() const;
 | 
					        const std::string getUrl() const;
 | 
				
			||||||
        const WebSocketPerMessageDeflateOptions& getPerMessageDeflateOptions() const;
 | 
					        const WebSocketPerMessageDeflateOptions getPerMessageDeflateOptions() const;
 | 
				
			||||||
        int getPingInterval() const;
 | 
					        int getPingInterval() const;
 | 
				
			||||||
        size_t bufferedAmount() const;
 | 
					        size_t bufferedAmount() const;
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -14,12 +14,12 @@ namespace ix
 | 
				
			|||||||
{
 | 
					{
 | 
				
			||||||
    /// Default values as defined in the RFC
 | 
					    /// Default values as defined in the RFC
 | 
				
			||||||
    const uint8_t WebSocketPerMessageDeflateOptions::kDefaultServerMaxWindowBits = 15;
 | 
					    const uint8_t WebSocketPerMessageDeflateOptions::kDefaultServerMaxWindowBits = 15;
 | 
				
			||||||
    static const int minServerMaxWindowBits = 8;
 | 
					    static const uint8_t minServerMaxWindowBits = 8;
 | 
				
			||||||
    static const int maxServerMaxWindowBits = 15;
 | 
					    static const uint8_t maxServerMaxWindowBits = 15;
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    const uint8_t WebSocketPerMessageDeflateOptions::kDefaultClientMaxWindowBits = 15;
 | 
					    const uint8_t WebSocketPerMessageDeflateOptions::kDefaultClientMaxWindowBits = 15;
 | 
				
			||||||
    static const int minClientMaxWindowBits = 8;
 | 
					    static const uint8_t minClientMaxWindowBits = 8;
 | 
				
			||||||
    static const int maxClientMaxWindowBits = 15;
 | 
					    static const uint8_t maxClientMaxWindowBits = 15;
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    WebSocketPerMessageDeflateOptions::WebSocketPerMessageDeflateOptions(
 | 
					    WebSocketPerMessageDeflateOptions::WebSocketPerMessageDeflateOptions(
 | 
				
			||||||
        bool enabled,
 | 
					        bool enabled,
 | 
				
			||||||
@@ -85,11 +85,7 @@ namespace ix
 | 
				
			|||||||
 | 
					
 | 
				
			||||||
            if (startsWith(token, "server_max_window_bits="))
 | 
					            if (startsWith(token, "server_max_window_bits="))
 | 
				
			||||||
            {
 | 
					            {
 | 
				
			||||||
                std::string val = token.substr(token.find_last_of("=") + 1);
 | 
					                uint8_t x = std::stoi(token.substr(token.find_last_of("=") + 1));
 | 
				
			||||||
                std::stringstream ss;
 | 
					 | 
				
			||||||
                ss << val;
 | 
					 | 
				
			||||||
                int x;
 | 
					 | 
				
			||||||
                ss >> x;
 | 
					 | 
				
			||||||
 | 
					
 | 
				
			||||||
                // Sanitize values to be in the proper range [8, 15] in
 | 
					                // Sanitize values to be in the proper range [8, 15] in
 | 
				
			||||||
                // case a server would give us bogus values
 | 
					                // case a server would give us bogus values
 | 
				
			||||||
@@ -99,11 +95,7 @@ namespace ix
 | 
				
			|||||||
 | 
					
 | 
				
			||||||
            if (startsWith(token, "client_max_window_bits="))
 | 
					            if (startsWith(token, "client_max_window_bits="))
 | 
				
			||||||
            {
 | 
					            {
 | 
				
			||||||
                std::string val = token.substr(token.find_last_of("=") + 1);
 | 
					                uint8_t x = std::stoi(token.substr(token.find_last_of("=") + 1));
 | 
				
			||||||
                std::stringstream ss;
 | 
					 | 
				
			||||||
                ss << val;
 | 
					 | 
				
			||||||
                int x;
 | 
					 | 
				
			||||||
                ss >> x;
 | 
					 | 
				
			||||||
 | 
					
 | 
				
			||||||
                // Sanitize values to be in the proper range [8, 15] in
 | 
					                // Sanitize values to be in the proper range [8, 15] in
 | 
				
			||||||
                // case a server would give us bogus values
 | 
					                // case a server would give us bogus values
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -39,8 +39,8 @@ namespace ix
 | 
				
			|||||||
        bool _enabled;
 | 
					        bool _enabled;
 | 
				
			||||||
        bool _clientNoContextTakeover;
 | 
					        bool _clientNoContextTakeover;
 | 
				
			||||||
        bool _serverNoContextTakeover;
 | 
					        bool _serverNoContextTakeover;
 | 
				
			||||||
        int _clientMaxWindowBits;
 | 
					        uint8_t _clientMaxWindowBits;
 | 
				
			||||||
        int _serverMaxWindowBits;
 | 
					        uint8_t _serverMaxWindowBits;
 | 
				
			||||||
 | 
					
 | 
				
			||||||
        void sanitizeClientMaxWindowBits();
 | 
					        void sanitizeClientMaxWindowBits();
 | 
				
			||||||
    };
 | 
					    };
 | 
				
			||||||
 
 | 
				
			|||||||
		Reference in New Issue
	
	Block a user