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.

Sign up for GitHub

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

Jump to bottom

fix: shut down executorService thread automatically to allow the JVM to shut down gracefully #502

Merged
merged 2 commits into from
Mar 5, 2020
Merged

fix: shut down executorService thread automatically to allow the JVM to shut down gracefully #502

merged 2 commits into from
Mar 5, 2020

Conversation

Salil999
Copy link
Contributor

@Salil999 Salil999 commented Dec 1, 2019 •
edited by thinkingserious
Loading

Fixes #431

Initial attempt at trying to mitigate some threading issues in the Twilio API. I'm a bit new to open source, but I think this should try and help (or at least pave the initial steps) for issue #431

I didn't quite see the reason why a boolean was necessary (as mentioned by @bohnman in #431). Regardless if the executor service is set by the user or if it is generated internally, we should shutdown the executor service anyway, am I right?

Oh, and I think there might have been a small typo in the setUsername method. I updated that as well.

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

@thinkingserious thinkingserious changed the title Thread fix fix: shutdown executorService thread automatically to allow the JVM to shut down gracefully Mar 5, 2020
@thinkingserious thinkingserious changed the title fix: shutdown executorService thread automatically to allow the JVM to shut down gracefully fix: shut down executorService thread automatically to allow the JVM to shut down gracefully Mar 5, 2020
@thinkingserious thinkingserious added difficulty: medium fix is medium in difficulty status: ready for merge code deemed fit for merge type: community enhancement feature request not on Twilio's roadmap type: bug bug in the library and removed type: community enhancement feature request not on Twilio's roadmap labels Mar 5, 2020
thinkingserious
thinkingserious approved these changes Mar 5, 2020
View reviewed changes
Copy link
Contributor

@thinkingserious thinkingserious left a comment

Choose a reason for hiding this comment

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

Thank you!

LGTM :shipit:

@thinkingserious thinkingserious merged commit c420ca3 into twilio:master Mar 5, 2020
@thinkingserious thinkingserious mentioned this pull request Mar 5, 2020
Closed
@Salil999 Salil999 deleted the thread_fix branch April 12, 2020 20:27
FalguniV pushed a commit to FalguniV/twilio-java that referenced this pull request Oct 13, 2020
@thinkingserious @FalguniV
fix: shut down executorService thread automatically to allow the JVM …
133117f 
…to shut down gracefully (twilio#502)
FalguniV pushed a commit to FalguniV/twilio-java that referenced this pull request Oct 13, 2020
@thinkingserious @FalguniV
fix: shut down executorService thread automatically to allow the JVM …
e890136 
…to shut down gracefully (twilio#502)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thinkingserious thinkingserious thinkingserious approved these changes

Assignees
No one assigned
Labels
difficulty: medium fix is medium in difficulty status: ready for merge code deemed fit for merge type: bug bug in the library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

executor service never gets cleaned up
2 participants
@Salil999 @thinkingserious