Skip to content

added setConnectionManager(bPooling) to SpotifyApi.Builder#375

Merged
dargmuesli merged 6 commits into
spotify-web-api-java:betafrom
gahisee:poolingConnectionManager
Jan 22, 2024
Merged

added setConnectionManager(bPooling) to SpotifyApi.Builder#375
dargmuesli merged 6 commits into
spotify-web-api-java:betafrom
gahisee:poolingConnectionManager

Conversation

@gahisee

@gahisee gahisee commented Dec 3, 2023

Copy link
Copy Markdown
Contributor

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

@dargmuesli dargmuesli left a comment

Copy link
Copy Markdown
Member

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 🙌

Comment thread .gitignore Outdated
* 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
Copy Markdown
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.

Comment thread src/main/java/se/michaelthelin/spotify/SpotifyHttpManager.java Outdated
Comment thread src/main/java/se/michaelthelin/spotify/SpotifyHttpManager.java Outdated
Comment thread src/main/java/se/michaelthelin/spotify/SpotifyHttpManager.java Outdated
Comment thread src/main/java/se/michaelthelin/spotify/SpotifyApi.java Outdated
Comment thread src/main/java/se/michaelthelin/spotify/SpotifyApi.java Outdated
Comment thread src/main/java/se/michaelthelin/spotify/SpotifyApi.java Outdated
Comment thread src/main/java/se/michaelthelin/spotify/SpotifyHttpManager.java Outdated
and eliminated setConnectionManager()
…lean

and restored previous SpotifyApi.build() with no parameter
Comment thread spotify-web-api-java.jar Outdated
Comment thread src/main/java/se/michaelthelin/spotify/SpotifyHttpManager.java Outdated
@dargmuesli

Copy link
Copy Markdown
Member

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

@ulpcan

ulpcan commented Dec 19, 2023

Copy link
Copy Markdown

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

@dargmuesli

dargmuesli commented Dec 30, 2023

Copy link
Copy Markdown
Member

@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
Copy Markdown
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

gahisee commented Dec 30, 2023

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
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
Copy Markdown
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