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

SQS SendMessageBatchResponse hasFailed method unexpected response (versions 2.21.18+) #4759

Closed
mihalyr opened this issue Dec 8, 2023 · 7 comments
Labels
bug This issue is a bug. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days.

Comments

@mihalyr
Copy link

mihalyr commented Dec 8, 2023

Describe the bug

Hi, I have just bumped from 2.21.17 to 2.21.18 and facing a similar issue in my tests as people reported in #4713

I tested that the latest 2.21.40 is also broken the same way.

On 2.21.2

image

image

On 2.21.40

image

image

This does not seem to be a server-side issue (I'm using Localstack) and it is fixed on the older SDK version and broken with the new one. I think the bug is that the new SDK returns an empty Collections$UnmodifiableRandomAccessList instance, while the old implementation was correctly returning DefaultSdkAutoConstructList which is then checked for in the method implementation:

    public final boolean hasFailed() {
        return failed != null && !(failed instanceof SdkAutoConstructList);
    }

Expected Behavior

SendMessageBatchResponse.hasFailed() used to return false when there were no failed items when sending the batch in version 2.21.17 or older.

Current Behavior

SendMessageBatchResponse.hasFailed() returns true when there were no failed items when sending the batch in version 2.21.18 or newer because failed item list changed from DefaultSdkAutoConstructList what the method expects to a different instance.

Reproduction Steps

Using 2.21.18 or newer including 2.21.40:

    final SendMessageBatchResponse response = sqsClient.sendMessageBatch(
          SendMessageBatchRequest.builder()
              .queueUrl(queueUrl)
              .entries(messages)
              .build()
      );

    assertThat(response.hasFailed()).isFalse();

Possible Solution

SDK should not change behavior between patch versions. The problem seems to be that the SDK somewhere started to return a different List instance for failed items which breaks the hasFailed method original implementations. A workaround is to check for isEmpty on the failed items as suggested by the Javadoc, which is OK, but not expected to have to fix broken usage on a patch release.

Additional Information/Context

No response

AWS Java SDK version used

2.21.18 and above including the latest 2.21.40

JDK version used

OpenJDK Runtime Environment Corretto-21.0.1.12.1 (build 21.0.1+12-LTS)

Operating System and version

Fedora 38

@mihalyr mihalyr added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 8, 2023
@mihalyr mihalyr changed the title (short issue description) SQS SendMessageBatchResponse hasFailed method unexpected response Dec 8, 2023
@mihalyr
Copy link
Author

mihalyr commented Dec 8, 2023

Ugh, ReceiveMessageResponse.hasMessages() is broken now the same way, it keeps returning true on 2.21.40 when there are no messages returned. This is the same issue for some reason the SDK switched to Collections$UnmodifiableRandomAccessList that broke all of these methods that check for SdkAutoConstructList implementations. I wonder how this release made it through the tests without breaking anything. Unless the special case for SdkAutoConstructList was never really supported?

image

@mihalyr
Copy link
Author

mihalyr commented Dec 8, 2023

I tracked it down, the problematic release is 2.21.18 and above. It still works on 2.21.17 but on anything above 2.21.18 breaks. I update the versions in the description,

@mihalyr mihalyr changed the title SQS SendMessageBatchResponse hasFailed method unexpected response SQS SendMessageBatchResponse hasFailed method unexpected response (2.21.18) Dec 8, 2023
@mihalyr mihalyr changed the title SQS SendMessageBatchResponse hasFailed method unexpected response (2.21.18) SQS SendMessageBatchResponse hasFailed method unexpected response (versions 2.21.18 to 2.21.40) Dec 8, 2023
@mihalyr mihalyr changed the title SQS SendMessageBatchResponse hasFailed method unexpected response (versions 2.21.18 to 2.21.40) SQS SendMessageBatchResponse hasFailed method unexpected response (versions 2.21.18+) Dec 8, 2023
@debora-ito
Copy link
Member

@mihalyr To make sure the issue is not with Localstack, can you test using the SQS endpoint directly?

@debora-ito debora-ito added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days. and removed needs-triage This issue or PR still needs to be triaged. labels Dec 8, 2023
@mihalyr
Copy link
Author

mihalyr commented Dec 8, 2023

You mean that Localstack is causing the SDK to use a different Java List implementation internally? Idk, but I can check against SQS to see how it works. Were you not able to test it based on the reproducer I provided?

@mihalyr
Copy link
Author

mihalyr commented Dec 8, 2023

Now, this is very interesting, it seems that my tests work against SQS, indeed!

The thing is that it broke all our integ tests that use Localstack :( I can't easily switch to run these tests on SQS and tracking down all the methods that changed behavior is also not very appealing. So, 2.21.17 works against both SQS and Localstack, but 2.21.18+ doesn't work anymore properly with the same Localstack.

Where do you think the problem should be fixed? Localstack maybe doing something different from the SQS service after the protocol change?

@mihalyr
Copy link
Author

mihalyr commented Dec 8, 2023

I just made sure that I use the latest Localstack image and still face the same problem. I believe it would be better to close this issue and create a new one for Localstack instead to see if they are able to fix it on their side. For the time being I will stick to the last working version 2.21.17

@mihalyr mihalyr closed this as completed Dec 8, 2023
Copy link

github-actions bot commented Dec 8, 2023

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days.
Projects
None yet
Development

No branches or pull requests

2 participants