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

[FLINK-32592] Fix (Stream)ExEnv#initializeContextEnvironment thread-safety #22997

Merged
merged 1 commit into from
Jul 19, 2023
Merged

[FLINK-32592] Fix (Stream)ExEnv#initializeContextEnvironment thread-safety #22997

merged 1 commit into from
Jul 19, 2023
+116 −4

Conversation

zentol
Copy link
Contributor

@zentol zentol commented Jul 14, 2023

If multiple threads call initializeContextEnvironment then they may interfere with the value stored in the thread local, because that is derived from a field in the environment, not the method argument.

@zentol zentol requested a review from dmvk July 14, 2023 18:02
@flinkbot
Copy link
Collaborator

flinkbot commented Jul 14, 2023 •
edited

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@zentol zentol changed the title [FLINK-32592] Jar run/plan handlers run sequentially [FLINK-32592] Fix (Stream)ExEnv#initializeContextEnvironment thread-safety Jul 17, 2023
dmvk
dmvk reviewed Jul 19, 2023
View reviewed changes
flink-java/src/test/java/org/apache/flink/api/java/ExecutionEnvironmentTest.java
class ExecutionEnvironmentTest {

@Test
void testConcurrentSetContext() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Is it correct that without the fix, the test wasn't failing reliably but was just flaky?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes!

dmvk
dmvk approved these changes Jul 19, 2023
View reviewed changes
Copy link
Member

@dmvk dmvk left a comment

Choose a reason for hiding this comment

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

Makes sense. LGTM

@dmvk
Copy link
Member

dmvk commented Jul 19, 2023

I'm curious about the fallback behavior in case you don't have thread local context. 🤔 Especially since the contextEnvironmentFactory is not marked as volatile. I guess the correct behavior would be simply using atomicReference there and only allow setting it once.

@zentol
Copy link
Contributor Author

zentol commented Jul 19, 2023

I'm curious about the fallback behavior in case you don't have thread local context.

An atomic reference or volatile field would be better than what exists right now. I don't know why the field wasn't removed when the thread local was introduced; maybe for backwards-compatibility where 1 thread sets the context but another runs the job? 🤷

My initial feeling was that this field should just be removed.

@zentol zentol merged commit 13d3536 into apache:master Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmvk dmvk dmvk approved these changes

Assignees
No one assigned
Labels
component=Client/JobSubmission
Projects
None yet
Milestone
No milestone
3 participants
@zentol @flinkbot @dmvk