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.

Sign up for GitHub

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

Jump to bottom

Don't delete QUIC stream until both read and write complete #11196

Merged
merged 2 commits into from Apr 1, 2024
Merged

Don't delete QUIC stream until both read and write complete #11196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
12a454d
Don't use reference for stream id
maskit Mar 28, 2024
c029419
Don't delete QUIC stream until both read and write complete
maskit Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions include/iocore/net/quic/QUICStreamManager.h
View file
Open in desktop
Expand Up @@ -47,9 +47,9 @@ class QUICStreamManager : public QUICStreamStateListener
QUICStream *find_stream(QUICStreamId stream_id);

QUICConnectionErrorUPtr create_stream(QUICStreamId stream_id);
QUICConnectionErrorUPtr create_uni_stream(QUICStreamId &new_stream_id);
QUICConnectionErrorUPtr create_bidi_stream(QUICStreamId &new_stream_id);
QUICConnectionErrorUPtr delete_stream(QUICStreamId &new_stream_id);
QUICConnectionErrorUPtr create_uni_stream(QUICStreamId new_stream_id);
QUICConnectionErrorUPtr create_bidi_stream(QUICStreamId new_stream_id);
QUICConnectionErrorUPtr delete_stream(QUICStreamId new_stream_id);
void reset_stream(QUICStreamId stream_id, QUICStreamErrorUPtr error);

void set_default_application(QUICApplication *app);
Expand Down
4 changes: 4 additions & 0 deletions include/iocore/net/quic/QUICStreamVCAdapter.h
View file
Open in desktop
Expand Up @@ -50,6 +50,10 @@ class QUICStreamVCAdapter : public VConnection, public QUICStreamAdapter
void do_io_shutdown(ShutdownHowTo_t howto) override;
void reenable(VIO *vio) override;

// Helpers to check VIO states
bool is_readable();
bool is_writable();

void clear_read_ready_event(Event *e);
void clear_read_complete_event(Event *e);
void clear_write_ready_event(Event *e);
Expand Down
2 changes: 2 additions & 0 deletions include/proxy/http3/Http3App.h
View file
Open in desktop
Expand Up @@ -69,10 +69,12 @@ class Http3App : public QUICApplication

private:
void _handle_uni_stream_on_read_ready(int event, VIO *vio);
void _handle_uni_stream_on_read_complete(int event, VIO *vio);
void _handle_uni_stream_on_write_ready(int event, VIO *vio);
void _handle_uni_stream_on_write_complete(int event, VIO *vio);
void _handle_uni_stream_on_eos(int event, VIO *vio);
void _handle_bidi_stream_on_read_ready(int event, VIO *vio);
void _handle_bidi_stream_on_read_complete(int event, VIO *vio);
void _handle_bidi_stream_on_write_ready(int event, VIO *vio);
void _handle_bidi_stream_on_write_complete(int event, VIO *vio);
void _handle_bidi_stream_on_eos(int event, VIO *vio);
Expand Down
6 changes: 3 additions & 3 deletions src/iocore/net/quic/QUICStreamManager.cc
View file
Open in desktop
Expand Up @@ -101,19 +101,19 @@ QUICStreamManager::create_stream(QUICStreamId stream_id)
}

QUICConnectionErrorUPtr
QUICStreamManager::create_uni_stream(QUICStreamId &new_stream_id)
QUICStreamManager::create_uni_stream(QUICStreamId new_stream_id)
{
return nullptr;
}

QUICConnectionErrorUPtr
QUICStreamManager::create_bidi_stream(QUICStreamId &new_stream_id)
QUICStreamManager::create_bidi_stream(QUICStreamId new_stream_id)
{
return nullptr;
}

QUICConnectionErrorUPtr
QUICStreamManager::delete_stream(QUICStreamId &stream_id)
QUICStreamManager::delete_stream(QUICStreamId stream_id)
{
QUICStream *stream = static_cast<QUICStream *>(this->find_stream(stream_id));

Expand Down
12 changes: 12 additions & 0 deletions src/iocore/net/quic/QUICStreamVCAdapter.cc
View file
Open in desktop
Expand Up @@ -328,6 +328,18 @@ QUICStreamVCAdapter::reenable(VIO *vio)
// until the application consume data.
}

bool
QUICStreamVCAdapter::is_readable()
{
return this->stream().direction() != QUICStreamDirection::SEND && _read_vio.nbytes == _read_vio.ndone;
}

bool
QUICStreamVCAdapter::is_writable()
{
return this->stream().direction() != QUICStreamDirection::RECEIVE && _write_vio.nbytes != -1;
}

int
QUICStreamVCAdapter::state_stream_open(int event, void *data)
{
Expand Down
48 changes: 35 additions & 13 deletions src/proxy/http3/Http3App.cc
View file
Open in desktop
Expand Up @@ -153,11 +153,10 @@ Http3App::main_event_handler(int event, Event *data)
break;
case VC_EVENT_READ_COMPLETE:
adapter->clear_read_complete_event(data);
// Calling read_ready handlers because there's no need to do different things
if (is_bidirectional) {
this->_handle_bidi_stream_on_read_ready(event, vio);
this->_handle_bidi_stream_on_read_complete(event, vio);
} else {
this->_handle_uni_stream_on_read_ready(event, vio);
this->_handle_uni_stream_on_read_complete(event, vio);
}
break;
case VC_EVENT_WRITE_READY:
Expand Down Expand Up @@ -282,6 +281,17 @@ Http3App::_handle_uni_stream_on_read_ready(int /* event */, VIO *vio)
}
}

void
Http3App::_handle_uni_stream_on_read_complete(int event, VIO *vio)
{
this->_handle_uni_stream_on_read_ready(event, vio);

QUICStreamVCAdapter *adapter = static_cast<QUICStreamVCAdapter *>(vio->vc_server);
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make sense to check for adapter != nullptr or maybe an assert?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's already checked in Http3App::main_event_handler.

if (!adapter->is_writable()) {
this->_qc->stream_manager()->delete_stream(adapter->stream().id());
}
}

void
Http3App::_handle_bidi_stream_on_read_ready(int event, VIO *vio)
{
Expand All @@ -304,6 +314,17 @@ Http3App::_handle_bidi_stream_on_read_ready(int event, VIO *vio)
}
}

void
Http3App::_handle_bidi_stream_on_read_complete(int event, VIO *vio)
{
this->_handle_bidi_stream_on_read_ready(event, vio);

QUICStreamVCAdapter *adapter = static_cast<QUICStreamVCAdapter *>(vio->vc_server);
if (!adapter->is_writable()) {
this->_qc->stream_manager()->delete_stream(adapter->stream().id());
}
}

void
Http3App::_handle_uni_stream_on_write_ready(int /* event */, VIO *vio)
{
Expand Down Expand Up @@ -343,9 +364,14 @@ Http3App::_handle_uni_stream_on_write_ready(int /* event */, VIO *vio)
}

void
Http3App::_handle_uni_stream_on_write_complete(int /* event */, VIO *vio)
Http3App::_handle_uni_stream_on_write_complete(int event, VIO *vio)
{
// QUICStreamVCAdapter *adapter = static_cast<QUICStreamVCAdapter *>(vio->vc_server);
this->_handle_uni_stream_on_write_ready(event, vio);

QUICStreamVCAdapter *adapter = static_cast<QUICStreamVCAdapter *>(vio->vc_server);
if (!adapter->is_readable()) {
this->_qc->stream_manager()->delete_stream(adapter->stream().id());
}
}

void
Expand Down Expand Up @@ -431,14 +457,10 @@ Http3App::_handle_bidi_stream_on_write_ready(int event, VIO *vio)
void
Http3App::_handle_bidi_stream_on_write_complete(int event, VIO *vio)
{
QUICStreamVCAdapter *adapter = static_cast<QUICStreamVCAdapter *>(vio->vc_server);
this->_handle_bidi_stream_on_write_ready(event, vio);

QUICStreamId stream_id = adapter->stream().id();
Http3Transaction *txn = static_cast<Http3Transaction *>(this->_ssn->get_transaction(stream_id));
if (txn != nullptr) {
SCOPED_MUTEX_LOCK(lock, txn->mutex, this_ethread());
txn->handleEvent(event);
QUICStreamVCAdapter *adapter = static_cast<QUICStreamVCAdapter *>(vio->vc_server);
if (!adapter->is_readable()) {
this->_qc->stream_manager()->delete_stream(adapter->stream().id());
}
// FIXME There may be data to read
this->_qc->stream_manager()->delete_stream(stream_id);
}