Skip to content
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

Jump to bottom

Use sessionRequest for wrapping HTTP stream instead of original Request #12303

Merged
merged 1 commit into from
Sep 26, 2024
Merged

Use sessionRequest for wrapping HTTP stream instead of original Request #12303

merged 1 commit into from
Sep 26, 2024

Conversation

robbie01
Copy link
Contributor

This fixes flushOnResponseCommit logic, which is currently broken due to SessionHandler.getManagedSession returning null. With this, getManagedSession will return the SessionRequest.

@janbartel
Copy link
Contributor

@robbie01 thanks for the PR. Can you please also modify the SessionHandlerTest so it reveals the problem (and tests that it's solved).

@janbartel janbartel self-assigned this Sep 24, 2024
@robbie01 robbie01 force-pushed the jetty-12.0.x branch 2 times, most recently from c5f5182 to ad7c02b Compare September 24, 2024 08:56
@robbie01
Copy link
Contributor Author

@janbartel Added a test case and squashed. Looks like it catches; it fails for request and succeeds for sessionRequest. Please let me know if anything else isn't up to the project's standards.

@janbartel janbartel mentioned this pull request Sep 24, 2024
Closed
@janbartel
Copy link
Contributor

@robbie01 this is looking good, thank you! It has even revealed a problem with the core OpenId stuff - whether that's a test setup issue or an implementation issue I'm not sure yet, but I've raised #12307. I think we might need to fix that first before we can commit your fix, otherwise we'll break the build. Stand by.

@janbartel janbartel added Bug For general bugs on Jetty side High Priority labels Sep 24, 2024
@janbartel
Copy link
Contributor

@robbie01 actually I discovered the problem was deeper than the OpenId stuff - there was a bug in core security SessionAuthentication: see #12309 and my fix in #12310. When I get that applied to jetty-12.0.x, can you rebase your PR please.

@robbie01
Copy link
Contributor Author

@janbartel Absolutely, I'll have that taken care of once it's ready.

@janbartel
Copy link
Contributor

@robbie01 ok, you're good to do a rebase now.

@robbie01
Use sessionRequest for wrapping HTTP stream instead of original Request.
fbb3e12 
Add SessionHandlerTest case to ensure that flushOnResponseCommit is not broken
in the future.

Signed-off-by: Robert B. Langer <robertlanger03@gmail.com>
@robbie01
Copy link
Contributor Author

@janbartel Done, thanks!

janbartel
janbartel approved these changes Sep 26, 2024
View reviewed changes
Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

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

Love it!

@janbartel janbartel merged commit 8aa9c8b into jetty:jetty-12.0.x Sep 26, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janbartel janbartel janbartel approved these changes

Assignees

@janbartel janbartel

Labels
Bug For general bugs on Jetty side High Priority
Projects
No open projects
Jetty 12.0.14
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@robbie01 @janbartel