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: Fix a possible NULL PTR after introduced timeout on waitForDone #1638

Merged
merged 8 commits into from
May 4, 2022
Merged

fix: Fix a possible NULL PTR after introduced timeout on waitForDone #1638

merged 8 commits into from
May 4, 2022

Conversation

yirutang
Copy link
Contributor

@yirutang yirutang commented May 3, 2022

This PR:
#1637

introduced a timeout for waitForDone. So we potentially could have drained the inflight requests when a RequestCallback comes back. Add process for null when peak the inflight queue.

@yirutang yirutang requested review from a team and prash-mi May 3, 2022 20:55
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: bigquerystorage Issues related to the googleapis/java-bigquerystorage API. labels May 3, 2022
@yirutang yirutang requested a review from gnanda May 3, 2022 20:55
@yirutang
Copy link
Contributor Author

yirutang commented May 3, 2022

@GaoleMeng

@@ -573,6 +573,9 @@ private void requestCallback(AppendRowsResponse response) {
conectionRetryCountWithoutCallback = 0;
}
requestWrapper = pollInflightRequestQueue();
if (requestWrapper == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we haven't cleaned up and this happens, we should throw an exception

Maybe instead of using null, check if we've cleaned up, and don't call the method (since this is under lock anyways)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we have cleaned up, then all inflight requests are all set with an exception already.
I don't think we should throw on requestCallback?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant the opposite: if we haven't cleaned up.

If during normal operations we get a callback but there's nothing in the queue, that should never happen, so we should throw (or somehow error out)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the code, hope it is more straightforward.

Copy link
Contributor

@gnanda gnanda May 3, 2022

Choose a reason for hiding this comment

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

Yeah it is, but I was thinking we'd also have

doneCallback {
this.done = true;
}

and for your change in requestCallback:
if (!this.done && this.inflightRequestQueue.isEmpty()) {
// Handle this error case!
}

But since it's in the callback, that's probably more complicated than I originally thought... Should we handle that in a followup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PTAL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no super good place for done, since the lock is done inside of the cleanupInflightQueue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an error message, without a response, user will probably not be able to get the actual exception.

@stephaniewang526
Copy link
Contributor

@yirutang
There is a lint issue on this PR: can be fixed by running mvn com.coveo:fmt-maven-plugin:format
And OwlBot is pending: can be fixed by tagging this PR owlbot: run
Then you should be able to merge it in (without my approval).

@yirutang yirutang added the owlbot:run Add this label to trigger the Owlbot post processor. label May 4, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 4, 2022
@yirutang yirutang merged commit e1c6ded into googleapis:main May 4, 2022
gcf-merge-on-green bot pushed a commit that referenced this pull request May 5, 2022
🤖 I have created a release *beep* *boop*
---


## [2.13.0](v2.12.2...v2.13.0) (2022-05-05)


### Features

* add support to a few more specific StorageErrors for the Write API ([#1563](#1563)) ([c26091e](c26091e))
* next release from main branch is 2.12.2 ([#1624](#1624)) ([b2aa2a4](b2aa2a4))


### Bug Fixes

* A stuck when the client fail to get DoneCallback ([#1637](#1637)) ([3baa84e](3baa84e))
* Fix a possible NULL PTR after introduced timeout on waitForDone ([#1638](#1638)) ([e1c6ded](e1c6ded))


### Dependencies

* update dependency com.google.cloud:google-cloud-bigquery to v2.10.10 ([#1623](#1623)) ([54b74b8](54b74b8))
* update dependency org.apache.avro:avro to v1.11.0 ([#1632](#1632)) ([b47eea0](b47eea0))


### Documentation

* **samples:** update WriteComittedStream sample code to match best practices ([#1628](#1628)) ([5d4c7e1](5d4c7e1))
* **sample:** update WriteToDefaultStream sample to match best practices ([#1631](#1631)) ([73ddd7b](73ddd7b))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquerystorage Issues related to the googleapis/java-bigquerystorage API. size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants