Skip to content

Check resourceManager state first in getMoreLoop#1439

Closed
rozza wants to merge 1 commit intomongodb:masterfrom
rozza:JAVA-5516
Closed

Check resourceManager state first in getMoreLoop#1439
rozza wants to merge 1 commit intomongodb:masterfrom
rozza:JAVA-5516

Conversation

@rozza
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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