Skip to content

Remove H3 frame sequence number tracking#11632

Merged
maskit merged 2 commits intoapache:masterfrom
maskit:h3_fix_protocol_enforcer
Jul 31, 2024
Merged

Remove H3 frame sequence number tracking#11632
maskit merged 2 commits intoapache:masterfrom
maskit:h3_fix_protocol_enforcer

Conversation

@maskit
Copy link
Copy Markdown
Member

@maskit maskit commented Jul 31, 2024

This closes #11622.

The frame sequence was only used for ProtocolEnforcer and there is no other use case planned. This PR removes the sequence number tracking, which was reset on every READ_READY, and use a private flag in ProtocolEnforcer to do the same check for SETTINGS frame.

@maskit maskit added the HTTP/3 label Jul 31, 2024
@maskit maskit added this to the 10.1.0 milestone Jul 31, 2024
@maskit maskit self-assigned this Jul 31, 2024
@maskit maskit requested a review from JosiahWI July 31, 2024 18:51
Copy link
Copy Markdown
Contributor

@JosiahWI JosiahWI left a comment

Choose a reason for hiding this comment

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

Nice cleanup! Let's not free the buffer in the test twice, though.

@JosiahWI
Copy link
Copy Markdown
Contributor

The tests for AggregateWriteBuffer segfaulted on CentOs. That is unrelated to this change and I will look into it.

@maskit
Copy link
Copy Markdown
Member Author

maskit commented Jul 31, 2024

@JosiahWI Do you want to gather information before I push a new commit and trigger new jenkins jobs?

@JosiahWI
Copy link
Copy Markdown
Contributor

@maskit I'm not sure I can gather anything useful from the job. I don't know how to get a stack trace. I suspect it's a race condition, but I don't know how it's related to the test or whether it's related at all. The valgrind report on my OpenSUSE container comes up completely clean.

@maskit
Copy link
Copy Markdown
Member Author

maskit commented Jul 31, 2024

Ok, I'l rerun the job without pushing a new commit to see if it fails again. I'll push a new commit if it doesn't fail.

@maskit
Copy link
Copy Markdown
Member Author

maskit commented Jul 31, 2024

[approve ci centos]

@maskit
Copy link
Copy Markdown
Member Author

maskit commented Jul 31, 2024

All tests passed this time.

Copy link
Copy Markdown
Contributor

@JosiahWI JosiahWI left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@maskit
Copy link
Copy Markdown
Member Author

maskit commented Jul 31, 2024

139/139 Test   #9: test_AIO ...............................***Timeout 1500.06 sec
configuration file: sample.cfg
reading disk_size = 1
reading hotset_size = 1
reading hotset_frequency = 0.5
reading run_time = 30
reading threads_per_disk = 1
reading touch_data = 1
reading seq_read_percent = 0.5
reading seq_write_percent = 0.3
reading rand_read_percent = 0.2
reading seq_read_size = 131072
reading seq_write_size = 4093
reading rand_read_size = 4096
reading write_skip = 5
reading chains = 1
reading delete_disks = 1
reading disk_path = ./aio.tst
reading num_processors = 5
Starting hotset document writing 
Finished hotset documents  [8] offset [  8192] size [131072]
Starting the aio_testing 
=================================================================
==4621==ERROR: AddressSanitizer: heap-use-after-free on address 0x6040000001b8 at pc 0x0000004530d4 bp 0x7f0593db2fa0 sp 0x7f0593db2f90
READ of size 1 at 0x6040000001b8 thread T3 ([ET_NET 2])

@maskit
Copy link
Copy Markdown
Member Author

maskit commented Jul 31, 2024

[approve ci rocky]

@maskit maskit merged commit aaff5a9 into apache:master Jul 31, 2024
@cmcfarlen cmcfarlen modified the milestones: 10.1.0, 10.0.0 Aug 1, 2024
@cmcfarlen
Copy link
Copy Markdown
Contributor

Cherry-picked to v10.0.x

cmcfarlen pushed a commit that referenced this pull request Aug 1, 2024
* Remove H3 frame sequence number tracking

* Remove redundant free_MIOBuffer call

(cherry picked from commit aaff5a9)
@maskit maskit deleted the h3_fix_protocol_enforcer branch September 18, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: picked-10.0.0

Development

Successfully merging this pull request may close these issues.

Http3FrameDispatcher counts sequence numbers incorrectly

3 participants