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 tiny_map from_iter where one operation was being dropped #2733

Merged
merged 3 commits into from
May 30, 2023

Conversation

Thor-Bjorgvinsson
Copy link
Contributor

@Thor-Bjorgvinsson Thor-Bjorgvinsson commented May 29, 2023

Motivation and Context

15th operation was being dropped when iterator was being read into a TinyMap

Description

The 15th operation was being dropped when the if statement was caught because the operation had been popped of the iterator but hadn't been added to the vec of operations before the two iterators were being chained and collected into a

Testing

Added unit tests that reproduced the issue and verified that the issue is fixed

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@Thor-Bjorgvinsson Thor-Bjorgvinsson requested review from a team as code owners May 29, 2023 17:02
@david-perez david-perez added server Rust server SDK bug Something isn't working labels May 29, 2023
Copy link
Contributor

@david-perez david-perez left a comment

Choose a reason for hiding this comment

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

Thanks!

CHANGELOG.next.toml Outdated Show resolved Hide resolved
Co-authored-by: david-perez <[email protected]>
@82marbag
Copy link
Contributor

@Thor-Bjorgvinsson you have probably ticked the wrong box in the checklist

@david-perez
Copy link
Contributor

@Velfi Seems like these tests are flaky. Although it's the first time I've seen them fail.

running 4 tests
test test_smol_runtime_timeouts ... FAILED
test test_async_std_runtime_timeouts ... FAILED
test test_smol_runtime_retry ... ok
test test_async_std_runtime_retry ... ok

failures:

---- test_smol_runtime_timeouts stdout ----
thread 'test_smol_runtime_timeouts' panicked at 'actual = 769.061231ms, expected = 350ms', sdk/s3/tests/alternative-async-runtime.rs:128:5

---- test_async_std_runtime_timeouts stdout ----
thread 'test_async_std_runtime_timeouts' panicked at 'actual = 778.09071ms, expected = 350ms', sdk/s3/tests/alternative-async-runtime.rs:128:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    test_async_std_runtime_timeouts
    test_smol_runtime_timeouts

test result: FAILED. 2 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.60s

@drganjoo drganjoo enabled auto-merge May 30, 2023 10:19
@drganjoo drganjoo disabled auto-merge May 30, 2023 10:21
@david-perez david-perez added this pull request to the merge queue May 30, 2023
Merged via the queue into smithy-lang:main with commit 5126a61 May 30, 2023
drganjoo pushed a commit that referenced this pull request May 30, 2023
15th operation was being dropped when iterator was being read into a
TinyMap

The 15th operation was being dropped when the if statement was caught
because the operation had been popped of the iterator but hadn't been
added to the vec of operations before the two iterators were being
chained and collected into a

Added unit tests that reproduced the issue and verified that the issue
is fixed

<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [ ] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
drganjoo pushed a commit that referenced this pull request May 30, 2023
15th operation was being dropped when iterator was being read into a
TinyMap

The 15th operation was being dropped when the if statement was caught
because the operation had been popped of the iterator but hadn't been
added to the vec of operations before the two iterators were being
chained and collected into a

Added unit tests that reproduced the issue and verified that the issue
is fixed

<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [ ] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
drganjoo added a commit that referenced this pull request May 30, 2023
15th operation was being dropped when iterator was being read into a
TinyMap

The 15th operation was being dropped when the if statement was caught
because the operation had been popped of the iterator but hadn't been
added to the vec of operations before the two iterators were being
chained and collected into a

Added unit tests that reproduced the issue and verified that the issue
is fixed

<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [ ] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._

Co-authored-by: Thor Bjorgvinsson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working server Rust server SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants