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

fix: make sure use of non-threadsafe UdfIndex is synchronized #3486

Merged
merged 1 commit into from
Oct 7, 2019

Conversation

purplefox
Copy link
Contributor

@purplefox purplefox commented Oct 7, 2019

Description

UdfIndex is not thread-safe, and is used in two place in the codebase where it could be accessed by different threads: UdfFactory and UdfAggregateFunctionFactory.

This PR adds synchronization to UdfFactory and `UdfAggregateFunctionFactory' to make this access thread-safe.

Currently addFunction is always called (in clock time) before getFunction. (Although this access pattern is not enforced in the class).

Note that this doesn't make the access safe, because there is no obvious happens-before relationship between the actions on a thread calling addFunction and another thread calling getFunction.

There could be an accidental happens-before due to the way threads are currently started in the server but relying on this is fragile. By synchronizing we guarantee a happens-before relationship between threads accessing these functions.

Testing done

Non functional change

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@purplefox purplefox requested a review from a team as a code owner October 7, 2019 06:43
Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

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

LGTM!

@agavra agavra requested a review from a team October 7, 2019 15:06
@purplefox purplefox merged commit 618aae8 into confluentinc:master Oct 7, 2019
@purplefox purplefox deleted the synchronize_UdfIndex branch October 28, 2019 00:21
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.

2 participants