Repository: envoyproxy/envoy
File: source/common/http/codec_client.h (C/C++)

Code before:
   * @param type supplies the codec type.
   * @param connection supplies the connection to communicate on.
   * @param host supplies the owning host.
   */
  CodecClient(CodecType type, Network::ClientConnectionPtr&& connection,
              Upstream::HostDescriptionConstSharedPtr host, Event::Dispatcher& dispatcher);

  // Http::ConnectionCallbacks
  void onGoAway(GoAwayErrorCode error_code) override {
    if (codec_callbacks_) {
      codec_callbacks_->onGoAway(error_code);
    }
  }
  void onSettings(ReceivedSettings& settings) override {
    if (codec_callbacks_) {
      codec_callbacks_->onSettings(settings);
    }
  }
  void onMaxStreamsChanged(uint32_t num_streams) override {
    if (codec_callbacks_) {
      codec_callbacks_->onMaxStreamsChanged(num_streams);
    }
  }

  void onIdleTimeout() {
    if (connected_) {
      host_->cluster().trafficStats()->upstream_cx_idle_timeout_.inc();
      close();
    }
  }

  void disableIdleTimer() {
    if (idle_timer_ != nullptr) {
      idle_timer_->disableTimer();
    }
  }

  void enableIdleTimer() {
    if (idle_timer_ != nullptr) {
      idle_timer_->enableTimer(idle_timeout_.value());
    }
  }

  const CodecType type_;
  // The order of host_, connection_, and codec_ matter as during destruction each can refer to
  // the previous, at least in tests.
  Upstream::HostDescriptionConstSharedPtr host_;
  Network::ClientConnectionPtr connection_;
  ClientConnectionPtr codec_;
  Event::TimerPtr idle_timer_;
  const absl::optional<std::chrono::milliseconds> idle_timeout_;

Review comment (suggestion):
This guard is good, but I think a better approach is don't let the idle timer fire at all while connecting. I’d prefer guard in `enableIdleTimer()` and we only call `enableIdldTimer()` when the connection is connected, and there are 2 places, rather than the existing approach during the active client construction. Once the upstream connection is connected, the upstream client will be moved to the `ready_clients_` to accept streams, so the idle timeout will be cleared / disabled at that time.

Code after:
   * @param connection supplies the connection to communicate on.
   * @param host supplies the owning host.
   */
  CodecClient(CodecType type, Network::ClientConnectionPtr&& connection,
              Upstream::HostDescriptionConstSharedPtr host, Event::Dispatcher& dispatcher);

  // Http::ConnectionCallbacks
  void onGoAway(GoAwayErrorCode error_code) override {
    if (codec_callbacks_) {
      codec_callbacks_->onGoAway(error_code);
    }
  }
  void onSettings(ReceivedSettings& settings) override {
    if (codec_callbacks_) {
      codec_callbacks_->onSettings(settings);
    }
  }
  void onMaxStreamsChanged(uint32_t num_streams) override {
    if (codec_callbacks_) {
      codec_callbacks_->onMaxStreamsChanged(num_streams);
    }
  }

  void onIdleTimeout() {
    host_->cluster().trafficStats()->upstream_cx_idle_timeout_.inc();
    close();
  }

  void disableIdleTimer() {
    if (idle_timer_ != nullptr) {
      idle_timer_->disableTimer();
    }
  }

  void enableIdleTimer() {
    // Bug fix (default): only enable idle timer when connection is established.
    // Old behavior (when flag is disabled): enable idle timer even when connection is not yet
    // established.
    if (!connected_ && enable_idle_timer_only_when_connected_) {
      return;
    }
    if (idle_timer_ != nullptr) {
      idle_timer_->enableTimer(idle_timeout_.value());
    }
  }

  const CodecType type_;
  // The order of host_, connection_, and codec_ matter as during destruction each can refer to
  // the previous, at least in tests.
  Upstream::HostDescriptionConstSharedPtr host_;
  Network::ClientConnectionPtr connection_;