Record HttpSM Id on new transaction #5766

masaori335 · 2019-08-05T06:23:39Z

After 0d2ad23 (#5747) merged, I intermittently faced below crash.

(lldb) bt
* thread #2, name = '[ET_NET 1]', stop reason = EXC_BAD_ACCESS (code=1, address=0x48)
  * frame #0: 0x000000010018efbc traffic_server`Http2Stream::send_request(this=0x000000010916fee0, cstate=0x00000001087fc040) at Http2Stream.cc:175:45
    frame #1: 0x0000000100185926 traffic_server`rcv_headers_frame(cstate=0x00000001087fc040, frame=0x0000000106c810f8) at Http2ConnectionState.cc:366:15
    frame #2: 0x000000010017bdde traffic_server`Http2ConnectionState::main_event_handler(this=0x00000001087fc040, event=2253, edata=0x0000000106c810f8) at Http2ConnectionState.cc:970:15
    frame #3: 0x0000000100010901 traffic_server`Continuation::handleEvent(this=0x00000001087fc040, event=2253, data=0x0000000106c810f8) at I_Continuation.h:190:12
    frame #4: 0x0000000100176a0c traffic_server`send_connection_event(cont=0x00000001087fc040, event=2253, edata=0x0000000106c810f8) at Http2ClientSession.cc:65:16
    frame #5: 0x0000000100178b4d traffic_server`Http2ClientSession::do_complete_frame_read(this=0x00000001087fbd20) at Http2ClientSession.cc:540:3
    frame #6: 0x0000000100178030 traffic_server`Http2ClientSession::state_process_frame_read(this=0x00000001087fbd20, event=100, vio=0x0000000107ab9d78, inside_frame=false) at Http2ClientSession.cc:584:5
    frame #7: 0x0000000100177c2a traffic_server`Http2ClientSession::state_start_frame_read(this=0x00000001087fbd20, event=100, edata=0x0000000107ab9d78) at Http2ClientSession.cc:470:10
    frame #8: 0x0000000100175a9a traffic_server`Http2ClientSession::main_event_handler(this=0x00000001087fbd20, event=100, edata=0x0000000107ab9d78) at Http2ClientSession.cc:344:22
    frame #9: 0x0000000100010901 traffic_server`Continuation::handleEvent(this=0x00000001087fbd20, event=100, data=0x0000000107ab9d78) at I_Continuation.h:190:12
    frame #10: 0x0000000100176836 traffic_server`Http2ClientSession::state_read_connection_preface(this=0x00000001087fbd20, event=100, edata=0x0000000107ab9d78) at Http2ClientSession.cc:450:20
    frame #11: 0x0000000100175a9a traffic_server`Http2ClientSession::main_event_handler(this=0x00000001087fbd20, event=100, edata=0x0000000107ab9d78) at Http2ClientSession.cc:344:22
    frame #12: 0x0000000100010901 traffic_server`Continuation::handleEvent(this=0x00000001087fbd20, event=100, data=0x0000000107ab9d78) at I_Continuation.h:190:12
    frame #13: 0x0000000100377ab8 traffic_server`read_signal_and_update(event=100, vc=0x0000000107ab9ba0) at UnixNetVConnection.cc:83:24
    frame #14: 0x0000000100377a4b traffic_server`UnixNetVConnection::readSignalAndUpdate(this=0x0000000107ab9ba0, event=100) at UnixNetVConnection.cc:1006:11
    frame #15: 0x0000000100331978 traffic_server`SSLNetVConnection::net_read_io(this=0x0000000107ab9ba0, nh=0x0000000105675d50, lthread=0x0000000105672000) at SSLNetVConnection.cc:644:11
    frame #16: 0x0000000100367e22 traffic_server`NetHandler::process_ready_list(this=0x0000000105675d50) at UnixNet.cc:396:11
    frame #17: 0x0000000100368871 traffic_server`NetHandler::waitForActivity(this=0x0000000105675d50, timeout=60000000) at UnixNet.cc:529:3
    frame #18: 0x00000001003a837b traffic_server`EThread::execute_regular(this=0x0000000105672000) at UnixEThread.cc:277:14
    frame #19: 0x00000001003a87e8 traffic_server`EThread::execute(this=0x0000000105672000) at UnixEThread.cc:338:11
    frame #20: 0x00000001003a6b42 traffic_server`spawn_thread_internal(a=0x0000000101524160) at Thread.cc:92:12
    frame #21: 0x00007fff7172c2eb libsystem_pthread.dylib`_pthread_body + 126
    frame #22: 0x00007fff7172f249 libsystem_pthread.dylib`_pthread_start + 66
    frame #23: 0x00007fff7172b40d libsystem_pthread.dylib`thread_start + 13

I thought accessing Http2Stream::current_reader in Http2Stream::send_request() is safe. But there is a path of Http2Stream::update_read_request() starts shutdown and make it nullptr.

maskit · 2019-08-05T07:18:03Z

proxy/http2/Http2Stream.cc

+  super::new_transaction();
+
+  ink_release_assert(this->current_reader != nullptr);
+  this->_http_sm_id = this->current_reader->sm_id;


Wouldn't it be needed by H3?

When H3 have Milestone/SlowLogs support.

Then why not put it to the parent class? Are we not going to support it?

The parent class is ProxyTransaction and it's also inherited by Http1Transaction. And Http1Transaction doesn't need this. Because Milestones for HTTP/1.1 is in HttpSM.
Milestones might be one thing of the refactoring SSN/TNX, but it's out of scope of this PR.

The parent class is ProxyTransaction and it's also inherited by Http1Transaction. And Http1Transaction doesn't need this. Because Milestones for HTTP/1.1 is in HttpSM.

This doesn't sounds like a good design. It's inconsistent. I don't think we should follow it, otherwise things get worse.

Milestones might be one thing of the refactoring SSN/TNX, but it's out of scope of this PR.

I understand this PR is trying to fix a crash, but it seems like to me that you are unnecessarily adding a thing need to be refactored.

If you disagree, I can live with it but you need to find someone else to get an approval.

I agree with Milestones, ProxyTransaction and HttpSM are not well designed.
But I don't want to do refactoring all of them here. Because 1) it might be huge changes 2) the refactoring HttpSM (#5488) is going on.

I'm fine with moving the assignment before the update_read_request() call instead of override new_transaction, if it looks simple.

Who said you should refactor everything?

Moving _http_sm_id to ProxyTransaction and initializing it in ProxyTransaction::new_transaction() are enough, no? Will it be a huge change? I think this is much simpler than overriding a method and it is also a right direction (no need to additional refactoring).

Now, I understand your suggestion. But I'm not big fun of adding a member which is not used by all child classes.

If we add the member on the ProxyTransaction, it's not used by Http1Transaction. Also if we derive Http(1|2|3)|ServerTransaction from ProxyTransaction, the _http_sm_id will not used by them. These relations of classes are really unclear for now.

If we add the member on the ProxyTransaction, it's not used by Http1Transaction.

Currently true, and I think it should be changed later. Otherwise having it in a parent class doesn't make sense. Many things for H1 is currently too special because of historical reasons. I think new things should be designed more generically. Do you want to add another _http_sm_id to H3Transaction and override new_transaction again? I hope you don't forget to override it.

Also if we derive Http(1|2|3)|ServerTransaction from ProxyTransaction, the _http_sm_id will not used by them. These relations of classes are really unclear for now.

I'd rename ProxyTransaction to ClientProxyTransaction and define a new ProxyTransaction that have common parts. I don't think _http_sm_id is the only member we don't need for server side.

There is a path of Http2Stream::update_read_request() starts shutdown and make current_reader nullptr. Copying HttpSM Id from current_reader should be done before the update_read_request() call.

masaori335 added the HTTP/2 label Aug 5, 2019

masaori335 added this to the 9.0.0 milestone Aug 5, 2019

masaori335 self-assigned this Aug 5, 2019

maskit reviewed Aug 5, 2019

View reviewed changes

masaori335 added the Crash label Aug 6, 2019

Record HttpSM Id on new transaction

63150aa

There is a path of Http2Stream::update_read_request() starts shutdown and make current_reader nullptr. Copying HttpSM Id from current_reader should be done before the update_read_request() call.

masaori335 force-pushed the h2-fix-sm-id branch from 3cc26bf to 63150aa Compare August 15, 2019 05:48

vmamidi self-requested a review August 15, 2019 07:00

vmamidi approved these changes Aug 15, 2019

View reviewed changes

vmamidi merged commit 296cf29 into apache:master Aug 15, 2019

masaori335 mentioned this pull request Mar 27, 2020

HTTP/2 backports for 8.1.x (part 1) #6581

Merged

zwoop modified the milestones: 9.0.0, 8.1.0 Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Record HttpSM Id on new transaction #5766

Record HttpSM Id on new transaction #5766

Uh oh!

masaori335 commented Aug 5, 2019

Uh oh!

maskit Aug 5, 2019

Uh oh!

masaori335 Aug 5, 2019

Uh oh!

maskit Aug 5, 2019

Uh oh!

masaori335 Aug 6, 2019

Uh oh!

maskit Aug 6, 2019

Uh oh!

masaori335 Aug 6, 2019

Uh oh!

maskit Aug 6, 2019

Uh oh!

masaori335 Aug 6, 2019

Uh oh!

maskit Aug 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Record HttpSM Id on new transaction #5766

Record HttpSM Id on new transaction #5766

Uh oh!

Conversation

masaori335 commented Aug 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants