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

Assign server response outside try block #17

Merged
merged 4 commits into from
Mar 9, 2017

Conversation

bluestealth
Copy link
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
Contributor

Hello @bluestealth,

Thanks for the PR!

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

@@ -277,23 +278,19 @@ public Response delete(Request request) throws URISyntaxException, IOException {
}

private Response executeApiCall(HttpRequestBase httpPost) throws IOException {
CloseableHttpResponse serverResponse = null;
Response response = new Response();
CloseableHttpResponse serverResponse = httpClient.execute(httpPost);
Copy link
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
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
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.

build.gradle Outdated
@@ -12,7 +12,7 @@
*/

apply plugin: 'java'
apply plugin: 'fatjar'
apply plugin: "eu.appsatori.fatjar"
Copy link
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
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