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

[UNDERTOW-1758] make state field atomic in HttpServerExchange #1560

Open
wants to merge 2 commits into
base: 2.2.x
Choose a base branch
from
Open

[UNDERTOW-1758] make state field atomic in HttpServerExchange #1560

wants to merge 2 commits into from

Conversation

burov4j
Copy link

@burov4j burov4j commented Feb 22, 2024

@fl4via fl4via added bug fix Contains bug fix(es) next release This PR will be merged before next release or has already been merged (for payload double check) waiting PR update Awaiting PR update(s) from contributor before merging labels Mar 15, 2024
carterkozak
carterkozak reviewed Mar 22, 2024
View reviewed changes
core/src/main/java/io/undertow/server/HttpServerExchange.java
@@ -480,9 +483,9 @@ public HttpServerExchange setRequestURI(final String requestURI) {
public HttpServerExchange setRequestURI(final String requestURI, boolean containsHost) {
this.requestURI = requestURI;
if (containsHost) {
this.state |= FLAG_URI_CONTAINS_HOST;
stateUpdater.accumulateAndGet(this, FLAG_URI_CONTAINS_HOST, (state, flag) -> state | flag);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps these lambdas could be extracted to static reusable instances -- sometimes lambdas like this in hot paths aren't elided as quickly as one might wish.

Copy link
Author

Choose a reason for hiding this comment

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

Isn't this a premature optimization?

@fl4via fl4via removed the next release This PR will be merged before next release or has already been merged (for payload double check) label Apr 19, 2024
@baranowb baranowb requested a review from ropalka June 24, 2024 10:56
@ropalka ropalka added backport The PR is the result of backporting another PR to a maintainance branch and removed waiting PR update Awaiting PR update(s) from contributor before merging labels Jun 25, 2024
@baranowb baranowb requested a review from fl4via September 4, 2024 07:05
@baranowb baranowb added the waiting PR update Awaiting PR update(s) from contributor before merging label Oct 30, 2024
@baranowb
Copy link
Contributor

@burov4j

  1. Could you please change "state" in updater invocation to "currentState" ? its kind of misleading(though in the end its the same ref).
  2. Could you please rebase and create PRs for main and 2.3.x?

@burov4j burov4j changed the base branch from 2.2.x to 2.3.x November 14, 2024 07:41
@burov4j burov4j changed the base branch from 2.3.x to 2.2.x November 14, 2024 07:45
@burov4j burov4j mentioned this pull request Nov 14, 2024
Closed
@burov4j
Copy link
Author

burov4j commented Nov 14, 2024

@baranowb looks like the issue was already fixed here: #1661

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

@carterkozak carterkozak carterkozak left review comments

@ropalka ropalka ropalka approved these changes

@baranowb baranowb baranowb approved these changes

@fl4via fl4via Awaiting requested review from fl4via

Assignees
No one assigned
Labels
backport The PR is the result of backporting another PR to a maintainance branch bug fix Contains bug fix(es) waiting PR update Awaiting PR update(s) from contributor before merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@burov4j @baranowb @ropalka @fl4via @carterkozak