Skip to content

Assign server response outside try block#17

Merged
thinkingserious merged 4 commits intosendgrid:masterfrom
bluestealth:master
Mar 9, 2017
Merged

Assign server response outside try block#17
thinkingserious merged 4 commits intosendgrid:masterfrom
bluestealth:master

Conversation

@bluestealth
Copy link
Copy Markdown
Contributor

@thinkingserious thinkingserious added type: bug bug in the library status: cla needed status: work in progress Twilio or the community is in the process of implementing labels Oct 27, 2016
@thinkingserious
Copy link
Copy Markdown
Contributor

Hello @bluestealth,

Thanks for the PR!

Could you please take a moment to sign our CLA so we can review?

Comment thread src/main/java/com/sendgrid/Client.java Outdated
private Response executeApiCall(HttpRequestBase httpPost) throws IOException {
CloseableHttpResponse serverResponse = null;
Response response = new Response();
CloseableHttpResponse serverResponse = httpClient.execute(httpPost);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't understand how this fixes sendgrid/sendgrid-java#165. It seems like the original code should be closing the connection at the end. Could you please elaborate on how your solution fixes the issue?

Some other questions:

  1. Did you test this? If so, could you describe them? I'd like to reproduce.
  2. By moving the httpClient.execute out of the try/catch/finally, how will we handle any exceptions at that function call.

Thanks!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are correct that this doesn't fix the error, and could result in some exceptional states not being caught, but I can imagine some sequence of events where serverResponse.close() isn't called with the existing code. Give me a bit to think about that.

With this finalizer as long as the client nulls their reference to Class SendGrid or the class goes out of scope the port should be released. I have tested this on my own machine by nulling my reference, forcing garbage collection and then busy waiting to keep the process open.

    @Override
    public void finalize() throws Throwable {
        try {
            this.httpClient.close();
        } catch(IOException e) {
            throw new Throwable(e.getMessage());
        } finally {
            super.finalize();
        }
    }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the follow up!

I'm not quite sure what is going on here, but I plan to investigate further.

Comment thread build.gradle Outdated

apply plugin: 'java'
apply plugin: 'fatjar'
apply plugin: "eu.appsatori.fatjar"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like this plugin is no longer under development, per: https://github.com/musketyr/gradle-fatjar-plugin

Do you mind converting to https://github.com/johnrengelman/shadow?

@thinkingserious thinkingserious merged commit 8f82b64 into sendgrid:master Mar 9, 2017
@thinkingserious
Copy link
Copy Markdown
Contributor

@bluestealth,

Thanks again for your support! Please take a moment to fill out this form so that we send you some swag :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: work in progress Twilio or the community is in the process of implementing type: bug bug in the library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sendgrid client doesn't close TCP connection properly

2 participants