Skip to content
This repository was archived by the owner on Aug 29, 2024. It is now read-only.

Add functionality to be able to peek n bytes off the wire#72

Merged
ioquatix merged 5 commits into
socketry:mainfrom
maruth-stripe:maruth-peek
Nov 9, 2023
Merged

Add functionality to be able to peek n bytes off the wire#72
ioquatix merged 5 commits into
socketry:mainfrom
maruth-stripe:maruth-peek

Conversation

@maruth-stripe

@maruth-stripe maruth-stripe commented Nov 6, 2023

Copy link
Copy Markdown
Contributor

To fix socketry/protocol-http2#14 , one approach is to maintain the invariant that the read buffer / wire is always frame-aligned. We propose doing this by always reading a full frame at a time. To do this, instead of reading data off the wire when reading the header for an H/2 frame, we just peek the header and then read the full frame.

Types of Changes

  • New feature.

Contribution

@maruth-stripe maruth-stripe marked this pull request as draft November 6, 2023 19:25
@ioquatix

ioquatix commented Nov 6, 2023

Copy link
Copy Markdown
Member

This is a great idea, I like it.

@ioquatix

ioquatix commented Nov 6, 2023

Copy link
Copy Markdown
Member

Do you mind adding some tests?

@maruth-stripe

Copy link
Copy Markdown
Contributor Author

Yup I'll get on it! thanks for the feedback

@ioquatix

ioquatix commented Nov 7, 2023

Copy link
Copy Markdown
Member

I'll also fix CI. Looks like there are some issues with Async v1 / Ruby 2.7.

@maruth-stripe maruth-stripe marked this pull request as ready for review November 7, 2023 01:38
@maruth-stripe

Copy link
Copy Markdown
Contributor Author

@ioquatix added a couple of tests!

@ioquatix

ioquatix commented Nov 7, 2023

Copy link
Copy Markdown
Member

Are you able to agree to the developer certificate of origin?

@maruth-stripe

Copy link
Copy Markdown
Contributor Author

@ioquatix yup did it! Sorry for multiple pushes, unable to get tests run locally due to environment issues

@maruth-stripe

Copy link
Copy Markdown
Contributor Author

ah managed to get tests running locally, they pass for me now!

@maruth-stripe

Copy link
Copy Markdown
Contributor Author

@ioquatix seems like the same (unrelated?) failures for 2.7/v1, any action needed from me?

@ioquatix

ioquatix commented Nov 9, 2023

Copy link
Copy Markdown
Member

No I will sort this out on the weekend, LGTM.

@ioquatix ioquatix merged commit 81c6728 into socketry:main Nov 9, 2023
dentarg added a commit to dentarg/sinatra that referenced this pull request Nov 11, 2023
Due to socketry/async-io#72 released in async-io 1.37.0

We will need this fix as long as we want to test Falcon in Ruby 2.6, as
the "fix" in async-io will be to raise the min Ruby version:
socketry/async-io#74
dentarg added a commit to sinatra/sinatra that referenced this pull request Nov 11, 2023
Due to socketry/async-io#72 released in async-io
1.37.0

We will need this fix as long as we want to test Falcon in Ruby 2.6, as
the "fix" in async-io will be to raise the min Ruby version:
socketry/async-io#74

Fail can be seen at
https://github.com/sinatra/sinatra/actions/runs/6836408460/job/18591270458#step:5:19
ioquatix added a commit that referenced this pull request Nov 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Framer's sequential reads of frame header then payload can leave underlying async stream's @read_buffer in a corrupted state

2 participants