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

RUST-1719 Fix panic after SessionCursor::with_type is called #928

Merged
merged 4 commits into from
Aug 14, 2023

Conversation

mlokr
Copy link
Contributor

@mlokr mlokr commented Aug 5, 2023

Hello there,

when using the transaction API, I encountered unconditional panic and also panic while dropping in SessionCursor::with_type. The problem occurs because the .state is moved into new cursor and subsequently, .unwrap() is called on it when marking it exhausted and also when checking for exhaustion. This is the minimal code change to fix this problem

@isabelatkinson
Copy link
Contributor

Hi @mlokr thank you for reporting this issue! Would you mind providing a reproduction of the code that caused this panic and let us know what version of the driver you're using? I will look into this in the meantime.

@isabelatkinson isabelatkinson self-requested a review August 10, 2023 15:19
Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

Update: I was able to reproduce this issue with the following test:

#[cfg_attr(feature = "tokio-runtime", tokio::test)]
#[cfg_attr(feature = "async-std-runtime", async_std::test)]
async fn session_cursor_with_type() {
    let _guard: RwLockReadGuard<()> = LOCK.run_concurrently().await;
    let client = TestClient::new().await;

    let mut session = client.start_session(None).await.unwrap();
    let coll = client.database("db").collection("coll");
    coll.drop_with_session(None, &mut session).await.unwrap();

    coll.insert_many_with_session(
        vec![doc! { "x": 1 }, doc! { "x": 2 }, doc! { "x": 3 }],
        None,
        &mut session,
    )
    .await
    .unwrap();

    let mut cursor: crate::SessionCursor<bson::Document> = coll
        .find_with_session(doc! {}, None, &mut session)
        .await
        .unwrap();

    let _ = cursor.next(&mut session).await.unwrap().unwrap();

    let mut cursor_with_type: crate::SessionCursor<bson::RawDocumentBuf> = cursor.with_type();

    let _ = cursor_with_type.next(&mut session).await.unwrap().unwrap();
}

Would you mind adding a test to verify the changes here fix this issue (feel free to paste in the one above)? (EDIT: the test can go in src/test/cursor.rs.)

Otherwise I just have one small comment!

src/cursor/session.rs Outdated Show resolved Hide resolved
@isabelatkinson isabelatkinson changed the title Fix session cursor retype RUST-1719 Fix panic after SessionCursor::with_type is called Aug 10, 2023
@mlokr
Copy link
Contributor Author

mlokr commented Aug 11, 2023

@isabelatkinson I added the test and made other requested changes, the newly added test passes for me. Thanks for providing it!

@isabelatkinson isabelatkinson force-pushed the fix-session-cursor-retype branch from fe26183 to 31bc1de Compare August 11, 2023 17:13
Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

Hey @mlokr, I rebased this onto main and fixed a small clippy failure. The changes look great! I am going to tag in the other member of the team for review; once he approves we'll merge this in and put out a patch release. Thanks again for bringing this bug to our attention!

@isabelatkinson isabelatkinson requested a review from abr-egn August 11, 2023 20:43
Copy link
Contributor

@abr-egn abr-egn left a comment

Choose a reason for hiding this comment

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

LGTM!

@isabelatkinson isabelatkinson merged commit 66f2904 into mongodb:main Aug 14, 2023
isabelatkinson added a commit to isabelatkinson/mongo-rust-driver that referenced this pull request Aug 15, 2023
…db#928)

Co-authored-by: Isabel Atkinson <isabel.atkinson@mongodb.com>
isabelatkinson added a commit that referenced this pull request Aug 15, 2023
Co-authored-by: Isabel Atkinson <isabel.atkinson@mongodb.com>
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.

3 participants