Fix TCP client seeing "already connected" error when resend message after having exception in the first send #94
Conversation
| this.endpoint = endpoint; | ||
| } | ||
|
|
||
| private void connect() { | ||
| try { | ||
| // Avoid "socket already connected" error (https://issues.amazon.com/issues/P54323886) |
There was a problem hiding this comment.
Remove the link from this comment. Also, the comment might not even be needed. If we keep it, you should be more descriptive as to why we want to create a new socket on every connect() call.
build.gradle
Outdated
| @@ -64,6 +64,7 @@ dependencies { | |||
| implementation 'com.fasterxml.jackson.core:jackson-annotations:2.11.1' | |||
| implementation 'com.fasterxml.jackson.datatype:jackson-datatype-jsr310:2.11.1' | |||
| implementation 'org.slf4j:slf4j-api:1.7.30' | |||
| implementation 'org.slf4j:slf4j-simple:1.7.30' | |||
There was a problem hiding this comment.
Without this, we'd see
software.amazon.cloudwatchlogs.emf.MetricsLoggerIntegrationTest > testMultipleFlushesOverTCP STANDARD_ERROR
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
There was a problem hiding this comment.
This is okay, we don't want or need to provide a logging implementation. User's provide their own logger.
| @@ -22,6 +23,7 @@ pushd $rootdir/src/integration-test/resources/agent | |||
| echo "[AmazonCloudWatchAgent] | |||
| aws_access_key_id = $AWS_ACCESS_KEY_ID | |||
| aws_secret_access_key = $AWS_SECRET_ACCESS_KEY | |||
| aws_session_token = $AWS_SESSION_TOKEN | |||
There was a problem hiding this comment.
Can you try running integration tests with an IAM user with no session token variable set, and see if this still works?
Also, we'll want this on master later as well.
There was a problem hiding this comment.
Okay cool. We still want to be using temporary credentials with a session token when possible, I just want to make sure it will still continue to work with long-lived credentials (if someone running the tests chooses to do that).
|
Can you make the PR title more descriptive please, describing more what this change is fixing/doing. |
caowei-amazon
changed the title
caowei-amazon
changed the title
| @@ -48,4 +48,50 @@ protected Socket createSocket() { | |||
|
|
|||
| assertEquals(bos.toString(), message); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testSendMessageWithGetOSException_THEN_createSocketTwice() throws IOException { | |||
There was a problem hiding this comment.
Once this PR is merged can we also add these tests to the master branch?
There was a problem hiding this comment.
Yes. I believe this also apply to that
Issue #, if available:
When TCP client sends a message and had exception, it will reuse the same socket to send another message which would cause error since the socket already in use.
Related ticket: https://issues.amazon.com/issues/P54323886
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.