Skip to content
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

Don't reset previously closed stream, since it's racy. #7922

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

yschimke
Copy link
Collaborator

@yschimke yschimke commented Jul 2, 2023

To address #7913

We could look at the nextStreamId and cancel after an unexpected data frame. But arguably we should terminate the connection if we are getting unexpected frames.

@yschimke
Copy link
Collaborator Author

yschimke commented Jul 2, 2023

Http2ConnectionTest[jvm] > bogusDataFrameDoesNotDisruptConnection()[jvm] FAILED
    java.util.concurrent.TimeoutException: bogusDataFrameDoesNotDisruptConnection() timed out after 5 seconds
        at org.junit.jupiter.engine.extension.TimeoutExceptionFactory.create(TimeoutExceptionFactory.java:29)
        at org.junit.jupiter.engine.extension.SameThreadTimeoutInvocation.proceed(SameThreadTimeoutInvocation.java:58)

@yschimke yschimke marked this pull request as draft July 2, 2023 12:55
@unlucku
Copy link

unlucku commented Oct 12, 2023

@yschimke I worked to the same solution as you for this bug and have been using it for 2 months without any issues.
Also, I dont think the connection should be terminated because the bug can occur in the following case as well.
(okhttp-5.0.0-alpha.11.jar)

private static void enableHttp2FrameLogging() {
		Logger frameLogger = Logger.getLogger(Http2.class.getName());
		frameLogger.setLevel(Level.FINE);
		ConsoleHandler handler = new ConsoleHandler();
		handler.setLevel(Level.FINE);
		handler.setFormatter(new SimpleFormatter() {
			@Override public String format(LogRecord record) {
				return String.format("%s%n", record.getMessage());
			}
		});
		frameLogger.addHandler(handler);
	}

	public static void main(String[] args) throws IOException {
		enableHttp2FrameLogging();
                // Start this executor service to prevent java from exiting. 
		Executors.newSingleThreadScheduledExecutor().schedule(() -> System.out.println(), 2, TimeUnit.MINUTES);
		Request r= new Request.Builder()
				...
				.build();
		try(Response res = new OkHttpClient().newCall(r).execute()) {
			System.out.println(res.code());
			return;
		}
	}

I think the code returns the response object when the headers frame is received by okhttp. In the code above, try catch closes the response object, which closes the stream.
Now in this instance, if a user is consuming the response body they aren't affected, since close will be called after the last (End_Stream flag) data frame is is received. However, if one just wants the response headers, like my code above, then there is a chance that the data frames arrive after close is called. This wouldn't really suggest any issue with the connection, since peer probably sent the frames before they received our RST_STREAM with cancel flag (Which is whats sent if we call close without consuming body).

The rfc says the following
However, after sending the RST_STREAM, the sending endpoint MUST be prepared to receive and process additional frames sent on the stream that might have been sent by the peer prior to the arrival of the RST_STREAM
https://www.rfc-editor.org/rfc/rfc9113.html#section-6.4-5

I think this change is perfect, I think chrome actually does something similar, where they dont send a RST_STREAM (Protocol_Error) for any data frames.


void SpdySession::OnStreamFrameData(spdy::SpdyStreamId stream_id,
                                    const char* data,
                                    size_t len) {
  CHECK(in_io_loop_);
  DCHECK_LT(len, 1u << 24);
  net_log_.AddEvent(NetLogEventType::HTTP2_SESSION_RECV_DATA, [&] {
    return NetLogSpdyDataParams(stream_id, len, false);
  });
  // Build the buffer as early as possible so that we go through the
  // session flow control checks and update
  // |unacked_recv_window_bytes_| properly even when the stream is
  // inactive (since the other side has still reduced its session send
  // window).
  std::unique_ptr<SpdyBuffer> buffer;
  if (data) {
    DCHECK_GT(len, 0u);
    CHECK_LE(len, static_cast<size_t>(kReadBufferSize));
    buffer = std::make_unique<SpdyBuffer>(data, len);
    DecreaseRecvWindowSize(static_cast<int32_t>(len));
    buffer->AddConsumeCallback(base::BindRepeating(
        &SpdySession::OnReadBufferConsumed, weak_factory_.GetWeakPtr()));
  } else {
    DCHECK_EQ(len, 0u);
  }
  auto it = active_streams_.find(stream_id);
  // By the time data comes in, the stream may already be inactive.
  if (it == active_streams_.end())
    return;
  SpdyStream* stream = it->second;
  CHECK_EQ(stream->stream_id(), stream_id);
  stream->AddRawReceivedBytes(len);
  stream->OnDataReceived(std::move(buffer));
}

https://source.chromium.org/chromium/chromium/src/+/main:net/spdy/spdy_session.cc;l=2836

Another thing that should be looked into is maybe moving the code in this pr to the reset stream function for Http2.Writer, as there might be other cases where we send reset stream errors for streams we have already closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants