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

do not use InetAddress in SocketAppenderBase #65

Open
SierraGolf opened this issue Mar 16, 2014 · 2 comments
Open

do not use InetAddress in SocketAppenderBase #65

SierraGolf opened this issue Mar 16, 2014 · 2 comments

Comments

@SierraGolf
Copy link
Contributor

The SocketAppenderBase uses an InetAddress to establish a socket connection. The object of the InetAddress is being created on setting the remoteHost string value. Since InetAddress does a host name lookup on initialization, it requires an internet connection, meaning even if the appender is being configured to be lazy initialized an application using such an appender would always need the device to be online in some way.

This can lead to different problems:

  1. The application might hang for some time until the device is online, in case it blocks on logger initialization and some weird android connectivity stuff is going on.
  2. Setting the address property might fail because of an non existing internet connection and cause the SocketAppender to never have an address, thus never being able to connect to a socket (silently returns on each connection attempt when address is null).

The same problem exists on the slightly different code on qos-ch/logback master. Their the problem can occur on starting the appender. I wonder what happens if an appender does not actually end up started after calling it's start method.

The easy solution for this problem would be to create the socket from the remoteHost and not use an InetAddress at all.

Probably there should also be an issue created on qos-ch/logback. @tony19 can you coordinate this? I think it might make sense to fix this just in one place and then merge it over.

@tony19
Copy link
Owner

tony19 commented Mar 17, 2014

logback-android is mostly sync'ed up with logback, so as of v1.1.1-1, SocketAppenderBase is now AbstractSocketAppender. It looks like I accidentally removed the lazy flag from SocketAppender in that merge.

But speaking in terms of v1.0.10-2 (which i think you're still using): I agree there's a problem with SocketAppenderBase#getRemoteHost(), regarding the lazy flag. It seems the address field should've been populated in SocketAppenderBase#start(). I do like the idea of removing the address field, since we only need the hostname and port to create the socket (both of which we already have).

Yes, please create the pull request in logback. Thanks!

@SierraGolf
Copy link
Contributor Author

Hi @tony19,

I gave this issue some more thought. Basically we want to do the following things:

  • not use INetAddress so we do not run in connection problems when we don't even try to create a connection yet
  • validate the host on start of the component

Both targets contradict. We can not make a host lookup without an internet connection. Meaning if we want to remove the dependency of an internet connection at start of a socket component it must not do a host lookup. We might just do a null/empty check and/or make the host lookup dependent on the lazy flag.

FYI: I can not see any lazy flags anymore in the socket components on your fork.

@tony19 tony19 added the bug label Jul 30, 2014
@stale stale bot added the wontfix label Mar 27, 2018
@tony19 tony19 added pinned and removed wontfix labels Mar 27, 2018
Repository owner deleted a comment from stale bot Mar 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants