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.

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

Reorganize local storing of Tx #1569

Open
wants to merge 15 commits into
base: hotfix-1.10.x
Choose a base branch
from
Open

Conversation

plyhun
Copy link
Contributor

@plyhun plyhun commented Dec 27, 2023

Abstract

One should never store a thing in ThreadLocal, since we operate async contexts, not threads.

Checklist

General

  • Added abstract that describes the change
  • Added changelog entry to /CHANGELOG.adoc
  • Ensured that the change is covered by tests
  • Ensured that the change is documented in the docs

On API Changes

  • Checked if the changes are breaking or not
  • Added GraphQL API if applicable
  • Added Elasticsearch mapping if applicable

Tx.threadLocalGraph.set(tx);
Optional.ofNullable(Vertx.currentContext())
.ifPresentOrElse(
ctx -> ctx.putLocal("tx", Optional.ofNullable(tx)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The actual change. There are get() and getLocal() in the Vert.x Context, which seem to be functionally equal, but getLocal() has internal usages.

return Tx.threadLocalGraph.get();
Optional<Context> maybeVertxContext = Optional.ofNullable(Vertx.currentContext());
if (maybeVertxContext.isPresent()) {
Context vertxContext = maybeVertxContext.get();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't mix the cases where we have an empty Context, and where we have no Context at all.

This reverts commit 2ed9611.
This reverts commit 13195ff.
This reverts commit 0cec36b.
This reverts commit 16a1700.
@plyhun
Copy link
Contributor Author

plyhun commented Jan 3, 2024

Postponed by now.

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.

1 participant