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

Leaking Two FD and one thread every time Options.refresh() is called #39

Open
joshgontijo opened this issue Oct 12, 2017 · 0 comments
Open

Comments

@joshgontijo
Copy link
Owner

joshgontijo commented Oct 12, 2017

From #148

There are no clean up code for the following so it is leaking a thread and two FD every time it is called.

syncIdleConnectionMonitorThread.start();

asyncConnectionManager = new PoolingNHttpClientConnectionManager(ioreactor);


One more thing. The following thread is never started for async.

setOption(Option.ASYNC_MONITOR, new AsyncIdleConnectionMonitorThread(asyncConnectionManager));


Hi, I'm also running in this, not with thousands of threads like reported in #140 but at least two. This can be reproduced by simply configuring an custom Object Mapper without invoking any further code
Unirest.setObjectMapper(new MyObjectMapper());
The static block inside Options with the refresh() causes an initial thread creation before setObjectMapper invokes refresh again. The previously created thread will not be removed.

I also think this whole static stuff is a design flaw (see #140) which should be replaced with a more object oriented interface like suggested by #145 and #138.


If you only call setProxy() or setTimeout() only once, you won't see this issue as much.

This is definitely a design issue. Since both setProxy() and setTimeout() are static, those settings can affect any running Thread calling Unirest. I believe the correct design would be having a factory method to create a specific Unirest instance per dependency. With the current design, the only way to avoid race condition is to load Unirest with custom classloader per dependency and encapsulate the custom class loader.

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

No branches or pull requests

1 participant