Skip to content

JAVA-3055 CqlPrepareAsyncProcessor must handle cancellations of the r… #1725

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

Closed
wants to merge 2 commits into from

Conversation

tomas-bilka
Copy link

@tomas-bilka tomas-bilka commented Aug 31, 2023

JAVA-3055
CqlPrepareAsyncProcessor must handle cancellations of the returned Future

@hhughes
Copy link
Contributor

hhughes commented Sep 7, 2023

Hi @tomas-bilka thank you for the PR.

Have you signed the Contributor License Agreement for contributions to DataStax open source projects? If not you can find it at https://cla.datastax.com/. Thanks!

@tomas-bilka
Copy link
Author

tomas-bilka commented Sep 7, 2023

I just signed the CLA.

@lucboutier
Copy link
Contributor

I made another PR for 3055 some time ago. Then had a review from a colleague for a better and simpler implementation which I believe is quite nice.

See
#1757

@mcoolive
Copy link

Any progress?

@aerain
Copy link

aerain commented Oct 2, 2024

I think this approach doesn't seem to be able to prevent the underlying problem.

There is edge case
When two duplicate prepare requests A and B are received (assuming A arrived slightly earlier), the cancellation of one request propagates to the other.

  1. Request A
  2. Cache loaded with request A. Asynchronous prepare call to Cassandra (referred to as 'a')
  3. Request B
  4. B waits for a response from the cache based on request A
  5. cancel() in request A
  6. B receives the cancel() from request A and propagates a CancellationException
  7. Response from 'a' completes. Upon receiving the cancellation of A, cache invalidation proceeds at this time

In my opinion, lucboutier's PR looks better to me.

@absurdfarce
Copy link
Contributor

I agree the defensive copy approach is probably a more robust way to fix this issue. Would be nice if we could more clearly disentangle the actors involved as well.

@absurdfarce
Copy link
Contributor

Closing in favor of #1757

@absurdfarce absurdfarce closed this Feb 3, 2025
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.

6 participants