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

Fix #1060 Enable Slack#close() method to shutdown thread pools behind its SlackConfig #1064

Merged
merged 2 commits into from
Sep 29, 2022

Conversation

seratch
Copy link
Member

@seratch seratch commented Sep 29, 2022

This pull request resolves #1060 by enhancing Slack and SlackConfig classes to shutdown thread pools once close() method is called. Also, I've updated the javadoc of the SlackConfig class to clearly mention that reusing the instance is recommended.

To reviewers:

Please don't feel pressured even if you are not familiar with the internals of this SDK. To learn how SlackConfig and its sub config classes are connected to the ThreadPools classes, you can check how ThreadPools.getOrCreate() method works.

Category (place an x in each of the [ ])

  • bolt (Bolt for Java)
  • bolt-{sub modules} (Bolt for Java - optional modules)
  • slack-api-client (Slack API Clients)
  • slack-api-model (Slack API Data Models)
  • slack-api-*-kotlin-extension (Kotlin Extensions for Slack API Clients)
  • slack-app-backend (The primitive layer of Bolt for Java)

Requirements

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you agree to those rules.

@seratch seratch added enhancement M-T: A feature request for new functionality project:slack-api-client project:slack-api-client labels Sep 29, 2022
@seratch seratch added this to the 1.26.0 milestone Sep 29, 2022
@seratch seratch self-assigned this Sep 29, 2022
@seratch
Copy link
Member Author

seratch commented Sep 29, 2022

The CI builds seem to be detecting some race condition issues:

closeMethod(test_locally.SlackConfigTest)  Time elapsed: 0.017 sec  <<< FAILURE!
java.lang.AssertionError: 
Expected: is <true>
     but: was <false>
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
	at test_locally.SlackConfigTest.closeMethod(SlackConfigTest.java:167)

@seratch
Copy link
Member Author

seratch commented Sep 29, 2022

^ the above issue was due to the test code quality. improved.

@codecov
Copy link

codecov bot commented Sep 29, 2022

Codecov Report

Merging #1064 (860be8a) into main (00c916e) will increase coverage by 0.01%.
The diff coverage is 81.48%.

@@             Coverage Diff              @@
##               main    #1064      +/-   ##
============================================
+ Coverage     76.78%   76.80%   +0.01%     
- Complexity     3718     3733      +15     
============================================
  Files           407      407              
  Lines         11197    11250      +53     
  Branches       1112     1128      +16     
============================================
+ Hits           8598     8640      +42     
- Misses         1928     1936       +8     
- Partials        671      674       +3     
Impacted Files Coverage Δ
...n/java/com/slack/api/methods/impl/ThreadPools.java 54.54% <46.66%> (-6.57%) ⬇️
...-api-client/src/main/java/com/slack/api/Slack.java 73.46% <50.00%> (-0.84%) ⬇️
...lient/src/main/java/com/slack/api/SlackConfig.java 38.66% <100.00%> (+2.11%) ⬆️
...ain/java/com/slack/api/audit/impl/ThreadPools.java 100.00% <100.00%> (ø)
...main/java/com/slack/api/scim/impl/ThreadPools.java 100.00% <100.00%> (ø)
...va/com/slack/api/socket_mode/SocketModeClient.java 63.69% <0.00%> (-0.64%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@WilliamBergamin WilliamBergamin left a comment

Choose a reason for hiding this comment

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

Nice use of AutoCloseable, this looks good to me 💯

Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

LGTM! I today learned about AutoCloseable 🙇

* In most use cases, this should not be intended. To avoid this, consider reusing SlackConfig objects
* as much as possible.
* <p>
* An alternative way is to set the statsEnabled flag to false.
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized that these docs are hosted on sonatype.org, and as far as I can tell, can't be previewed locally in Jekyll 😢 .
I wanted to check to see if the references to various classes (i.e. SlackConfig) and maybe even properties (i.e. statsEnabled) would link properly. I suppose we won't be able to tell until we merge + deploy?

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI: You can package javadoc.jar files by running mvn command and can unzip the file to check the generated HTML!

@seratch seratch merged commit 505d437 into slackapi:main Sep 29, 2022
@seratch seratch deleted the issue-1060-config branch September 29, 2022 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality project:slack-api-client project:slack-api-client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable Slack#close() method to shutdown thread pools behind its SlackConfig
3 participants