Skip to content

Return null when do_io_write called on closed stream #6826

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

Merged
shinrich merged 1 commit into from
Jun 10, 2020
Merged

Return null when do_io_write called on closed stream #6826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions proxy/http/HttpTunnel.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -956,6 +956,9 @@ HttpTunnel::producer_run(HttpTunnelProducer *p)
// Start the writes now that we know we will consume all the initial data
c->write_vio = c->vc->do_io_write(this, c_write, c->buffer_reader);
ink_assert(c_write > 0);
if (c->write_vio == nullptr) {
Copy link
Contributor

@masaori335 masaori335 May 28, 2020

Choose a reason for hiding this comment

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

Actually, I'm a bit surprised that we haven't had this check and haven't seen any problems. It looks like this case could happen with HTTP/1.1. Probably, Http2Stream easily falls down this case because of the life cycle.

consumer_handler(VC_EVENT_ERROR, c);
}
}
}
if (p->alive) {
Expand Down
8 changes: 6 additions & 2 deletions proxy/http2/Http2Stream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,8 +331,12 @@ Http2Stream::do_io_write(Continuation *c, int64_t nbytes, IOBufferReader *abuffe
write_vio.op = VIO::WRITE;
response_reader = abuffer;

update_write_request(abuffer, nbytes, false);

if (c != nullptr && nbytes > 0 && this->is_client_state_writeable()) {
update_write_request(abuffer, nbytes, false);
} else if (!this->is_client_state_writeable()) {
Copy link
Contributor

@masaori335 masaori335 May 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Are you assuming when HttpTunnel::producer_run() calls this do_io_write(), the Http2Stream is closing / closed ?
In that case, we should just return nullptr in the beginning of this function which what UnixNetVConnection does, I think.

trafficserver/iocore/net/UnixNetVConnection.cc

Lines 606 to 609 in fe90339

if (closed && !(c == nullptr && nbytes == 0 && reader == nullptr)) {
Error("do_io_write invoked on closed vc %p, cont %p, nbytes %" PRId64 ", reader %p", this, c, nbytes, reader);
return nullptr;
}

Copy link
Member Author

@shinrich shinrich May 27, 2020

Choose a reason for hiding this comment

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

Yes, that was my assumption, and this does look like a better solution. I will give that one a try.

// Cannot start a write on a closed stream
return nullptr;
}
return &write_vio;
}

Expand Down