Skip to content

Add finalize() and AutoCloseable support to AblyRest instances#807

Merged
QuintinWillison merged 8 commits intomainfrom
feature/executor-shutdown
Jun 22, 2022
Merged

Add finalize() and AutoCloseable support to AblyRest instances#807
QuintinWillison merged 8 commits intomainfrom
feature/executor-shutdown

Conversation

@QuintinWillison
Copy link
Contributor

A dual pronged approach to resolve #801.

Either:

  • The addition of finalize() implementation in f29e497 will passively clean up the thread pool executor once GC catches cleans up the AsyncHttpScheduler instance (once it's no longer referenced by the AblyRest instance that created it)
  • The adoption of AutoCloseable in 56593e5 will allow app developers to explicitly call close() on their AblyRest instances, or use try-with-resources

@QuintinWillison QuintinWillison self-assigned this Jun 20, 2022
@QuintinWillison QuintinWillison marked this pull request as ready for review June 21, 2022 10:06
Base automatically changed from feature/java-version to main June 21, 2022 13:39
Copy link
Contributor

@KacperKluka KacperKluka left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@ikbalkaya ikbalkaya left a comment

Choose a reason for hiding this comment

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

I made a comment on a line, otherwise looks good.

@QuintinWillison
Copy link
Contributor Author

I'm going to move this PR back to Draft state while I work out why the integration tests are failing in the REST suite. It is failing for me locally, possibly as a result of an unrelated move from Java 8 to 11 locally, but that's TBC. Sorry for the noise.

@QuintinWillison QuintinWillison marked this pull request as draft June 22, 2022 08:59
@QuintinWillison QuintinWillison marked this pull request as draft June 22, 2022 08:59
@QuintinWillison
Copy link
Contributor Author

We've identified that there has been a regression in the sandbox test environment. Specifically, proven using this command:

curl http://sandbox-rest.ably.io/stats -u <key>

Where it currently returns results, instead of exhibit expected behaviour which is to fail with:

Invalid use of Basic authentication over non-TLS transport.

As such, any integration test failures at the moment where they are only from the io.ably.lib.test.rest.RestAuthTest.auth_basic_nontls method are temporary and unrelated to the changes made in this PR.

@QuintinWillison QuintinWillison marked this pull request as ready for review June 22, 2022 12:56
@QuintinWillison QuintinWillison merged commit 282d2a4 into main Jun 22, 2022
@QuintinWillison QuintinWillison deleted the feature/executor-shutdown branch June 22, 2022 13:05
@QuintinWillison QuintinWillison mentioned this pull request Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Threads remain in parked (waiting) state indefinitely when AblyRest instance is freed

5 participants