Skip to content

Check resourceManager state first in getMoreLoop #1439

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
Closed
rozza wants to merge 1 commit into from
Closed

Check resourceManager state first in getMoreLoop #1439

rozza wants to merge 1 commit into from

Conversation

@rozza
Copy link
Member

@rozza rozza commented Jul 8, 2024

JAVA-5516

Notes:

I have not been able to replicate this error:

java.lang.AssertionError: null
	at com.mongodb.assertions.Assertions.assertTrue(Assertions.java:158)
	at com.mongodb.internal.operation.CursorResourceManager.setServerCursor(CursorResourceManager.java:224)
	at com.mongodb.internal.operation.AsyncCommandBatchCursor.lambda$getMoreLoop$2(AsyncCommandBatchCursor.java:186)

The stacktrace shows that the getMoreLoop method errored when returning results from the cursor because when setting the cursor via setServerCursor the following assertion failed: assertTrue(state.inProgress());.

The only methods that manipulate the state flag are:

  • tryStartOperation sets state iff state == IDLE
  • endOperation sets state if state == OPERATION_IN_PROGRESS or CLOSE_PENDING
  • close sets state if state == OPERATION_IN_PROGRESS to CLOSE_PENDING otherwise sets it to CLOSE

In this change I ensure that state is checked within a lock and reduced some of the excess assertions when setting the cursor.

@rozza rozza requested review from a team and stIncMale and removed request for a team July 8, 2024 14:31
@stIncMale
Copy link
Member

stIncMale commented Jul 8, 2024 •
edited
Loading

While I don't yet know what's wrong with the current code (before this PR), I am very much sure that we should neither remove the assertion, nor introduce locking to setServerCursor:

  • resourceManager.setServerCursor is called only as a result of getMoreLoop, which is called only as a result of next;
  • therefore, when resourceManager.setServerCursor is called, state.inProgress() indeed must be true, and anything else suggests a bug in the code, but not in the assertion itself;
  • additionally, the thread-safety of setServerCursor is to be guaranteed by the fact that it is called between (in HB) resourceManager.tryStartOperation and resourceManager.endOperation, so no locking should be needed.

@rozza I clearly remember reviewing a large PR created by you, where part of QueryBatchCursor.ResourceManager was refactored into a separate class (so that it could be used in AsyncQueryBatchCursor), and leaving quite a few comments there. However, I was unable to find that PR, and neither of the PRs linked in JAVA-5159 (#1246 and #1198) is the one I reviewed. I am guessing, that PR was closed without merging, but do you remember it at all, and are you able to locate it?

@stIncMale
Copy link
Member

stIncMale commented Jul 9, 2024

I was able to reproduce an error similar to the one reported in JAVA-5516, and fix the bug that is causing the error I observed. @rozza, could you take a look at #1440 and the comments there?

@stIncMale stIncMale mentioned this pull request Jul 9, 2024
Merged
@rozza
Copy link
Member Author

rozza commented Jul 9, 2024

@stIncMale - yeah I recall there being many reviews of the original work on my repo culminating here: rozza/pull/404

Closing in favor of #1440

@rozza rozza closed this Jul 9, 2024
@stIncMale
Copy link
Member

stIncMale commented Jul 9, 2024

yeah I recall there being many reviews of the original work on my repo culminating here: rozza#404

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stIncMale stIncMale Awaiting requested review from stIncMale stIncMale was automatically assigned from mongodb/dbx-java

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rozza @stIncMale