Skip to content

Removal of mutable reference from sni fn parameter - #3015

Open
satsharm wants to merge 1 commit into
rwf2:masterfrom
satsharm:master
Open

satsharm wants to merge 1 commit into
rwf2:masterfrom
satsharm:master

Conversation

@satsharm

@satsharm satsharm commented Oct 6, 2026 •

Copy link
Copy Markdown

Why this PR is needed?
sni function is supposed to provide the value from request and if we pass mut ref as parameter then it is bound to change the value of sni which is wrong so I removed mut ref from sni function parameter.

@satsharm satsharm changed the title removal of mutable reference from sni fn parameter Removal of mutable reference from sni fn parameter Oct 6, 2026
@hcldan

hcldan commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

I believe this is also preventing us from being able to use it in a handler where the reference to the request is not mutable.

@hcldan

hcldan commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

@the10thWiz I believe you did a lot of this work to get the feature into Rocket (thank you, btw). Could you please review this PR?

@HarrierOnChain

Copy link
Copy Markdown

I don't think this PR is needed, ty anyway!

@martynp

martynp commented Oct 6, 2026

Copy link
Copy Markdown

This exposes the sni in on_request fairings - which can be useful.

More importantly, I don't think this breaks anything.

@hcldan

hcldan commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

@martynp This doesn't expose anything new, it changes the existing exposure to be non-mutable so that we can access it when we do not have a mutable reference to the request.

I don't believe the intent was to allow this to be changeable after it is set by the rocket internals.

@hcldan

hcldan commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

I don't think this PR is needed, ty anyway!

@HarrierOnChain Can you please elaborate? We would not have offered this PR if we knew we were able to proceed without it.

@martynp

martynp commented Oct 6, 2026

Copy link
Copy Markdown

@martynp This doesn't expose anything new, it changes the existing exposure to be non-mutable so that we can access it when we do not have a mutable reference to the request.

I don't believe the intent was to allow this to be changeable after it is set by the rocket internals.

This has been requested in the past, there is an open PR with it somewhere.

As on_request cannot modify the request it is not currently available to on_request fairings.

Knowing the sni in an on_request fairing is useful.

As this change would make something available which was previously not available I used the term 'expose'. I am not sure I am incorrect in that regard.

@hcldan

hcldan commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

@martynp This doesn't expose anything new, it changes the existing exposure to be non-mutable so that we can access it when we do not have a mutable reference to the request.
I don't believe the intent was to allow this to be changeable after it is set by the rocket internals.

This has been requested in the past, there is an open PR with it somewhere.

As on_request cannot modify the request it is not currently available to on_request fairings.

Knowing the sni in an on_request fairing is useful.

As this change would make something available which was previously not available I used the term 'expose'. I am not sure I am incorrect in that regard.

Thanks for the clarification! Agreed.

@hcldan

hcldan commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

@SergioBenitez as this is a tiny change, do you think it could be merged soon?

@SergioBenitez

Copy link
Copy Markdown
Member

@hcldan In theory yes. I am working on a bunch of changes for a v0.6-rc release, which includes a change to the HTTP3 support, which touches this. As long as that doesn't conflict, I will merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants