Skip to content

RUST-384 Close connection which was dropped during command execution #214

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
Merged
merged 1 commit into from
Aug 3, 2020
Merged

RUST-384 Close connection which was dropped during command execution #214

merged 1 commit into from
Aug 3, 2020

Conversation

Lesiuk
Copy link
Contributor

@Lesiuk Lesiuk commented Jul 11, 2020

No description provided.

@saghm
Copy link
Contributor

saghm commented Jul 13, 2020

Thanks for the PR! It looks like the rustfmt lint check failed; could you add the nightly version of rustfmt (rustup component add rustfmt --toolchain nightly) and run rustfmt +nightly --unstable-features src/lib.rs? You can verify locally that it worked by running the .evergreen/check-rustfmt.sh script from the root of the repo.

saghm
saghm reviewed Jul 13, 2020
View reviewed changes
@Lesiuk
Copy link
Contributor Author

Lesiuk commented Jul 20, 2020

@saghm Reformatted and verified locally using check-rustfmt.sh script.

patrickfreed
patrickfreed reviewed Jul 21, 2020
View reviewed changes
Copy link
Contributor

@patrickfreed patrickfreed left a comment

Choose a reason for hiding this comment

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

Looks really good! I just have a few very minor nitpicks.

saghm
saghm reviewed Jul 29, 2020
View reviewed changes
Copy link
Contributor

@saghm saghm left a comment

Choose a reason for hiding this comment

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

@patrickfreed I've pushed to the branch with the updates you suggested

patrickfreed
patrickfreed approved these changes Jul 29, 2020
View reviewed changes
Copy link
Contributor

@patrickfreed patrickfreed left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @Lesiuk for your contribution!

isabelatkinson
isabelatkinson approved these changes Jul 29, 2020
View reviewed changes
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.

minor typo, but otherwise looks good!

src/cmap/conn/mod.rs Outdated
/// Whether or not a command is currently being run on this connection. This is set to `true`
/// right before sending bytes to the server and set back to `false` once a full response has
/// been read.
commmand_executing: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: there's an extra 'm' in command

@saghm saghm merged commit 9fa1655 into mongodb:master Aug 3, 2020
@saghm
Copy link
Contributor

saghm commented Aug 3, 2020

I just merged this; thanks again for identifying the bug and contributing the fix!

@ghost
Copy link

ghost commented Aug 14, 2020 •
edited by ghost
Loading

Do you have an eta for next crate.io release?
Currently 1.1.0-beta is published but it doesn't contain this fix. A 1.1.0-beta2 would be good too, but this fix is really critical. The driver can't be used to build an async web server in its current state since this bug is arising too frequently. Using patch in Cargo.toml is an option, but most users won't be necessarily aware of the fix.

@extrawurst
Copy link

extrawurst commented Aug 14, 2020 •
edited
Loading

@bcortier-devolutions I agree it is very essential, also I found another one related to this that popped up when switching to master: #234

all in all I am right now forced to work with a pinned down version of the driver to be stable

@ghost
Copy link

ghost commented Aug 14, 2020 •
edited by ghost
Loading

@extrawurst wow, thank you for the pointer on your PR. We're currently working with a pinned version too.

@Lesiuk Lesiuk deleted the RUST-384 branch August 14, 2020 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbroadst mbroadst mbroadst left review comments

@saghm saghm saghm approved these changes

@patrickfreed patrickfreed patrickfreed approved these changes

@isabelatkinson isabelatkinson isabelatkinson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Lesiuk @saghm @extrawurst @mbroadst @patrickfreed @isabelatkinson