Skip to content

Conversation

@gahisee
Copy link
Contributor

@gahisee gahisee commented Dec 3, 2023

if bPooling is true, PoolingHttpClientConnectionManager is used.
if not called, default is BasicHttpClientConnectionManager.
This addition allows for certain applications where multiple concurrent requests are made.
Otherwise, the following exception will occur:
java.lang.IllegalStateException: Connection 192.168.1.13:35776<->35.186.224.25:443 is still allocated
at org.apache.hc.core5.util.Asserts.check(Asserts.java:50)

in SpotifyApi; if not called, default is BasicHttpClientConnectionManager
Copy link
Member

@dargmuesli dargmuesli 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 for your suggestion! Some code changes would need to be done though 🙌

* A HttpManager Builder ready to build SpotifyHttpManager
*/
public static final IHttpManager DEFAULT_HTTP_MANAGER = new SpotifyHttpManager.Builder().build();
public static final SpotifyHttpManager.Builder shmb = new SpotifyHttpManager.Builder();
Copy link
Member

Choose a reason for hiding this comment

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

Please try to name variables without abbreviations.

and eliminated setConnectionManager()
…lean

and restored previous SpotifyApi.build() with no parameter
@dargmuesli
Copy link
Member

Please respond to the threads when making changes, indicating which threads can be resolved so I can verify and resolve 🙌

@ulpcan
Copy link

ulpcan commented Dec 19, 2023

I am getting this error too much, looking good. When do you plan to release?

@dargmuesli
Copy link
Member

dargmuesli commented Dec 30, 2023

@gahisee what do you think about just always using the pooling connection manager?

We could maybe save on some complexity by just using that instead of the basic connection manager, or do you see any breaking changes?

@dargmuesli
Copy link
Member

@ulpcan have you ever tried out the changes proposed in this PR in a project of yours or similar to confirm them to be working as is or have you just approved the changes without checking?

@gahisee
Copy link
Contributor Author

gahisee commented Dec 30, 2023

@dargmuesli i always like having options. With one call, the user can opt for pooling manager;
otherwise, default is the basic one-connection manager as always. If we always use the pooling manager, presumably there would be a bigger memory impact though likely inconsequential.

@Selbi182
Copy link
Member

Just wanna drop in and say I'm getting the same issue. It doesn't seem to affect the uptime of my app, but it does result in a lot of console spam:

grafik

@dargmuesli
Copy link
Member

See #384/files for my proposal, based on and including this PR, on how to implement the fix using the builder pattern.

@dargmuesli dargmuesli changed the base branch from master to beta January 22, 2024 13:43
@dargmuesli dargmuesli merged commit c04b762 into spotify-web-api-java:beta Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: allow to set a connection manager

4 participants