Skip to content

Conversation

@masaori335
Copy link
Contributor

Fix #7603.

The read from the client is enabled to watch the client abort, but this makes ATS signals EOS event to the HttpSM and goes HttpSM::state_watch_for_client_abort() handler.

// Enable further IO to watch for client aborts
ua_entry->read_vio->reenable();

When HttpSM sets up a HttpTunnelConsumer for a client, we can disable read to avoid the issue because there is nothing to read from the client. The EOS is handled by write op and signaled to HttpTunnel correctly.

@masaori335 masaori335 added this to the 10.0.0 milestone Mar 16, 2021
@masaori335 masaori335 self-assigned this Mar 16, 2021
Copy link
Member

@shinrich shinrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@bryancall
Copy link
Contributor

Is there a reason we don't handle the EOS appropriately? Do we also disable reads from the client in the HTTP/2 case? If so, then how does that work for bidirectional communication with HTTP/2?

@masaori335
Copy link
Contributor Author

Is there a reason we don't handle the EOS appropriately?

The HttpSM is in the state_watch_for_client_abort state and it kills HttpTunnel on EOS if ua_txn->allow_half_open() returns false. Checking what HttpTunnel is doing might be another approach, but I'm not sure it's better than this.

case VC_EVENT_EOS: {
// We got an early EOS.
NetVConnection *netvc = ua_txn->get_netvc();
if (ua_txn->allow_half_open()) {
if (netvc) {
netvc->do_io_shutdown(IO_SHUTDOWN_READ);
}
ua_entry->eos = true;
} else {
ua_txn->do_io_close();
ua_buffer_reader = nullptr;
vc_table.cleanup_entry(ua_entry);
ua_entry = nullptr;
tunnel.kill_tunnel();
terminate_sm = true; // Just die already, the requester is gone
set_ua_abort(HttpTransact::ABORTED, event);
}
break;
}

Do we also disable reads from the client in the HTTP/2 case? If so, then how does that work for bidirectional communication with HTTP/2?

With HTTP/2, this change disables the read of Http2Stream (just resets Http2Stream::read_vio which is used for receiving a request from the client on the stream).
The read op of HTTP/2 connection is still available.

@masaori335 masaori335 marked this pull request as draft March 29, 2021 01:25
@masaori335
Copy link
Contributor Author

masaori335 commented Mar 29, 2021

We tried this patch (on top of 8.1.2) and observed

  1. background fill is working well
  2. fewer client aborts
  3. +2K active client connections

The 3. might be a side effect of disabling read. I'll try another approach.

@masaori335
Copy link
Contributor Author

It turns out #7641 is a better approach because of fewer side effects.

@masaori335 masaori335 closed this Mar 30, 2021
@masaori335 masaori335 removed this from the 10.0.0 milestone Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

background_fill is broken with HTTP/1.1 over TLS

3 participants