Browse Source

Code review changes

Use optional int for maxOutstandingPings
change default value to existing behaviour without ping checking
rename to pingInterval
Tom Deblauwe 3 years ago
parent
commit
bfc0e94c16
4 changed files with 18 additions and 19 deletions
  1. 3 2
      include/rtc/websocket.hpp
  2. 2 2
      src/impl/websocket.cpp
  3. 8 10
      src/impl/wstransport.cpp
  4. 5 5
      src/impl/wstransport.hpp

+ 3 - 2
include/rtc/websocket.hpp

@@ -46,8 +46,9 @@ public:
 		bool disableTlsVerification = false; // if true, don't verify the TLS certificate
 		optional<ProxyServer> proxyServer;   // unsupported for now
 		std::vector<string> protocols;
-		int maxMissedPongsAllowed = 2;                   // -1 is no check
-		std::chrono::milliseconds sendPingInterval = std::chrono::seconds(10); // interval at which to send pings
+		optional<int> maxOutstandingPings;
+		std::chrono::milliseconds pingInterval =
+			std::chrono::seconds(10); // interval at which to send pings
 	};
 
 	WebSocket();

+ 2 - 2
src/impl/websocket.cpp

@@ -114,7 +114,7 @@ void WebSocket::open(const string &url) {
 
 	changeState(State::Connecting);
 	auto tcpTransport = std::make_shared<TcpTransport>(hostname, service, nullptr);
-	tcpTransport->setReadTimeout(config.sendPingInterval);
+	tcpTransport->setReadTimeout(config.pingInterval);
 	setTcpTransport(tcpTransport);
 }
 
@@ -363,7 +363,7 @@ shared_ptr<WsTransport> WebSocket::initWsTransport() {
 
 		auto transport = std::make_shared<WsTransport>(
 			lower, mWsHandshake, weak_bind(&WebSocket::incoming, this, _1), stateChangeCallback,
-			config.maxMissedPongsAllowed);
+			config.maxOutstandingPings);
 
 		return emplaceTransport(this, &mWsTransport, std::move(transport));
 

+ 8 - 10
src/impl/wstransport.cpp

@@ -54,7 +54,7 @@ using random_bytes_engine =
 
 WsTransport::WsTransport(variant<shared_ptr<TcpTransport>, shared_ptr<TlsTransport>> lower,
                          shared_ptr<WsHandshake> handshake, message_callback recvCallback,
-                         state_callback stateCallback, int maxPongsMissed)
+                         state_callback stateCallback, std::optional<int> maxOutstandingPings)
     : Transport(std::visit([](auto l) { return std::static_pointer_cast<Transport>(l); }, lower),
                 std::move(stateCallback)),
       mHandshake(std::move(handshake)),
@@ -62,7 +62,7 @@ WsTransport::WsTransport(variant<shared_ptr<TcpTransport>, shared_ptr<TlsTranspo
           std::visit(rtc::overloaded{[](shared_ptr<TcpTransport> l) { return l->isActive(); },
                                      [](shared_ptr<TlsTransport> l) { return l->isClient(); }},
                      lower)),
-      mMaxPongsMissed(maxPongsMissed) {
+      mMaxPongsMissed(maxOutstandingPings) {
 
 	onRecv(std::move(recvCallback));
 
@@ -133,7 +133,7 @@ void WsTransport::incoming(message_ptr message) {
 					PLOG_DEBUG << "WebSocket sending ping";
 					uint32_t dummy = 0;
 					sendFrame({PING, reinterpret_cast<byte *>(&dummy), 4, true, mIsClient});
-					addOpenPing();
+					addOutstandingPing();
 				} else {
 					Frame frame;
 					while (size_t len = readFrame(mBuffer.data(), mBuffer.size(), frame)) {
@@ -313,14 +313,12 @@ void WsTransport::recvFrame(const Frame &frame) {
 	}
 	case PING: {
 		PLOG_DEBUG << "WebSocket received ping, sending pong";
-		if (!sendFrame({PONG, frame.payload, frame.length, true, mIsClient})) {
-			PLOG_ERROR << "WebSocket could not send ping";
-		}
+		sendFrame({PONG, frame.payload, frame.length, true, mIsClient});
 		break;
 	}
 	case PONG: {
 		PLOG_DEBUG << "WebSocket received pong";
-		mPingsOpen = 0;
+		mPingsOutstanding = 0;
 		break;
 	}
 	case CLOSE: {
@@ -375,9 +373,9 @@ bool WsTransport::sendFrame(const Frame &frame) {
 	return outgoing(make_message(frame.payload, frame.payload + frame.length)); // payload
 }
 
-void WsTransport::addOpenPing() {
-	++mPingsOpen;
-	if (mMaxPongsMissed > 0 && mPingsOpen > mMaxPongsMissed) {
+void WsTransport::addOutstandingPing() {
+	++mPingsOutstanding;
+	if (mMaxPongsMissed && *mMaxPongsMissed > 0 && mPingsOutstanding > *mMaxPongsMissed) {
 		changeState(State::Failed);
 	}
 }

+ 5 - 5
src/impl/wstransport.hpp

@@ -33,8 +33,8 @@ class TlsTransport;
 class WsTransport final : public Transport {
 public:
 	WsTransport(variant<shared_ptr<TcpTransport>, shared_ptr<TlsTransport>> lower,
-	            shared_ptr<WsHandshake> handshake, message_callback recvCallback,
-				state_callback stateCallback, int maxPongsMissed);
+				shared_ptr<WsHandshake> handshake, message_callback recvCallback,
+				state_callback stateCallback, optional<int> maxOutstandingPings);
 	~WsTransport();
 
 	void start() override;
@@ -71,16 +71,16 @@ private:
 	void recvFrame(const Frame &frame);
 	bool sendFrame(const Frame &frame);
 
-	void addOpenPing();
+	void addOutstandingPing();
 
 	const shared_ptr<WsHandshake> mHandshake;
 	const bool mIsClient;
-	const int mMaxPongsMissed;
+	const optional<int> mMaxPongsMissed;
 
 	binary mBuffer;
 	binary mPartial;
 	Opcode mPartialOpcode;
-	int mPingsOpen = 0;
+	int mPingsOutstanding = 0;
 };
 
 } // namespace rtc::impl