Java (android) socket reconnection

[artik]

Hi,
Could somebody help me resolve I think small problem for You but huge for me.
How to correctly support reconnecting client sockets.
I have one thread (it's name thrd1) for controlling connection (and reconnection if is it needed) and thread (it's name thrd2) for cyclical sending data to server (it is written in Delhi).
Some strange happends when I stop server and start it again after some time.
Server receives information that my client (java-android) wants to connect several times (it depends on time - how long server doesn't respond) - but after these attempts connection back to almost normal state.
Problem is in these too many attempts in connection and in this that when time not responding of server is enough long my application crushes.

In my opinion problem is in "not-cleaning" socket after my stop method? How to do it perfectly?

Could somebody help me to resolve my huge problem?
Regards
Artik

If I can I put my example code:
thrd2 = new Thread(new Runnable() {
public void run() {
while (!Thread.interrupted()) {
try {
if (sock != null) {
out.write("TEST DATA\n");
out.flush();
try {
Thread.sleep(1000);
} catch (InterruptedException e) {
e.printStackTrace();
}
}
//if sock is null wait 300ms
else {
try {
Thread.sleep(300);
} catch (InterruptedException e) {
e.printStackTrace();
}
}
} catch (IOException e) {
try {
sock=null;
Thread.sleep(1000);
} catch (InterruptedException e1) {
e.printStackTrace();
}
}
}
}


and


thrd1 = new Thread(new Runnable() {
public void run() {
while (!Thread.interrupted()) {
try {
Thread.sleep(100);
} catch (InterruptedException e1) {

}
if (sock==null)
try {
sock = new Socket();
sock.connect(new InetSocketAddress(
address, 5000), 300);
r = new BufferedReader(new InputStreamReader(
sock.getInputStream()));
out = new BufferedWriter(
new OutputStreamWriter(sock
.getOutputStream()));
if ((thrd2!=null)&&(!thrd2.isAlive()))
thrd2.start();
} catch (UnknownHostException e) {
e.printStackTrace();

} catch (IOException e) {
e.printStackTrace();
}
}
;
}
});
if ((thrd1!=null)&&(!thrd1.isAlive())) thrd1.start();

 

[Daniele Futtorovic]

Hi,
Could somebody help me resolve I think small problem for You but huge for me.
How to correctly support reconnecting client sockets.
I have one thread (it's name thrd1) for controlling connection (and reconnection if is it needed) and thread (it's name thrd2) for cyclical sending data to server (it is written in Delhi).
Some strange happends when I stop server and start it again after some time.
Server receives information that my client (java-android) wants to connect several times (it depends on time - how long server doesn't respond) - but after these attempts connection back to almost normal state.
Problem is in these too many attempts in connection and in this that when time not responding of server is enough long my application crushes.

In my opinion problem is in "not-cleaning" socket after my stop method? How to do it perfectly?

Could somebody help me to resolve my huge problem?
Regards
Artik

If I can I put my example code:
thrd2 = new Thread(new Runnable() {
public void run() {
while (!Thread.interrupted()) {
try {
if (sock != null) {
out.write("TEST DATA\n");
out.flush();
try {
Thread.sleep(1000);
} catch (InterruptedException e) {
e.printStackTrace();
}
}
//if sock is null wait 300ms
else {
try {
Thread.sleep(300);
} catch (InterruptedException e) {
e.printStackTrace();
}
}
} catch (IOException e) {
try {
sock=null;
Thread.sleep(1000);
} catch (InterruptedException e1) {
e.printStackTrace();
}
}
}
}


and


thrd1 = new Thread(new Runnable() {
public void run() {
while (!Thread.interrupted()) {
try {
Thread.sleep(100);
} catch (InterruptedException e1) {

}
if (sock==null)
try {
sock = new Socket();
sock.connect(new InetSocketAddress(
address, 5000), 300);
r = new BufferedReader(new InputStreamReader(
sock.getInputStream()));
out = new BufferedWriter(
new OutputStreamWriter(sock
.getOutputStream()));
if ((thrd2!=null)&&(!thrd2.isAlive()))
thrd2.start();
} catch (UnknownHostException e) {
e.printStackTrace();

} catch (IOException e) {
e.printStackTrace();
}
}
;
}
});
if ((thrd1!=null)&&(!thrd1.isAlive())) thrd1.start();

`sock.close()' instead of `sock = null' would be a start... although
it's difficult to tell, as your code isn't complete.

It seems to me you have another problem in thrd1, though. If
sock.connect throws an Exception (for instance because the server isn't
available) on the first run of thrd1, then sock will be non-null
indefinitely, as far as I can see, even though it's not connected (thrd2
isn't started in that case). Looks like a design problem to me.

BTW, don't you think you could do the whole routine in but one thread?
The separation seems rather clunky.

 

[Eric Sosman]

Hi,
Could somebody help me resolve I think small problem for You but huge for me.
How to correctly support reconnecting client sockets.
I have one thread (it's name thrd1) for controlling connection (and reconnection if is it needed) and thread (it's name thrd2) for cyclical sending data to server (it is written in Delhi).
Some strange happends when I stop server and start it again after some time.
Server receives information that my client (java-android) wants to connect several times (it depends on time - how long server doesn't respond) - but after these attempts connection back to almost normal state.
Problem is in these too many attempts in connection and in this that when time not responding of server is enough long my application crushes.

In my opinion problem is in "not-cleaning" socket after my stop method? How to do it perfectly?

Could somebody help me to resolve my huge problem?
[... code snipped; see up-thread ...]

One problem (there may be others) is that your two threads
both use the `sock' variable without synchronization. Let's
call the threads T (the "transmitter") and C (the "connector"),
and imagine the following scenario:

T: writes to `sock', gets I/O error
T: sets `sock' null in response to the error
C: sees `sock' null, sets `sock = new Socket()'
C: performs `sock.connect()'
T: sees `sock' non-null and writes to it

That's presumably what you intended, but since there's no
synchronization something like this might happen instead:

T: writes to `sock', gets I/O error
T: sets `sock' null in response to the error
C: sees `sock' null, sets `sock = new Socket()'
T: sees `sock' non-null, writes, gets error (not connected)
T: sets `sock' null in response to second error
C: calls `sock.connect()' and gets NullPointerException

The problem is that either thread can try to use `sock'
while the other is in the middle of a sequence of steps that
make `sock' temporarily unusable. You must use synchronization
to ensure that neither thread touches `sock' while the other
is manipulating it. (You must use synchronization for some
other, subtler reasons, too.)

Among the other possible problems: It seems odd that after
an I/O error you just abandon the Socket without calling close()
on it. This looks very much like a resource leak that could
eventually bring down your program even if nothing else does.
(Or maybe not: I haven't studied it deeply.)

 

[artik]

My whole code looks like. I have to use to threads due to manage obtaining data from server too. Could You look at it in free time and tell me what's wrong?

Regards
Artik

public class MainActivity extends Activity {
Button button;
TextView textview1;
public Socket sock = null;
private BufferedWriter out;
private Thread thrd2, thrd1;
private static String address = "xyz";

@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
setContentView(R.layout.activity_main);
button = (Button) findViewById(R.id.button1);
textview1 = (TextView) findViewById(R.id.editText1);
button.setOnClickListener(new Button.OnClickListener() {

public void onClick(View v) {
thrd2 = new Thread(new Runnable() {
public void run() {
while (!Thread.interrupted()) {
try {
if (out != null) {
out.write("TEST DATA\n");
out.flush();
try {
Thread.sleep(1000);
} catch (InterruptedException e) {

e.printStackTrace();
}
}
// if sock is null wait 300ms
else {
try {
Thread.sleep(300);
} catch (InterruptedException e) {
e.printStackTrace();
}

}

} catch (IOException e) {
try {
stopSocket();
Thread.sleep(1000);
} catch (InterruptedException e1) {
e.printStackTrace();
}
}
}
}
});

thrd1 = new Thread(new Runnable() {
public void run() {
while (!Thread.interrupted()) {
try {
Thread.sleep(1000);
} catch (InterruptedException e1) {

}
if (sock == null)
try {
sock = new Socket();
sock.connect(new InetSocketAddress(address,
5000), 1000);
sock.setSoLinger(true, 1);
sock.setTcpNoDelay(true);
out = new BufferedWriter(
new OutputStreamWriter(sock
.getOutputStream()));
if ((thrd2 != null) && (!thrd2.isAlive()))
thrd2.start();
} catch (UnknownHostException e) {
stopSocket();

} catch (IOException e) {
stopSocket();
}
}
;
}
});

if ((thrd1 != null) && (!thrd1.isAlive()))
thrd1.start();
};
});
}

public void stopSocket() {
try {
if (out != null) {
out.close();
}
} catch (IOException e) {
Log.d("-----", e.getMessage() + " " + e.getCause());
}
try {
if (sock != null) {
sock.close();
}
sock = null;
} catch (IOException e) {
Log.d("-----", e.getMessage() + " " + e.getCause());
sock = null;
out = null;

}
}

}

 

[artik]

I'm sorry for my answer - it was mistake. I was very suprised than I've got interessted people my problem: You and Daniele. I wanted to answer as soon as possible and I have choosen wrong post.
Very, very sorry.
I try to improve my code using Your advice.

Artik

 

[artik]

Thank You for Your answer. I've placed my code by mistake in answer for Eric Sosman.

Regards
Artik

 

[Daniele Futtorovic]

My whole code looks like. I have to use to threads due to manage obtaining data from server too. Could You look at it in free time and tell me what's wrong?

<snip />

Artik,

Below you'll find a bunch of code relating to your issues. It is not
intended to be usable as such, but rather to give you some ideas about
how you /could/ organise things, as well as addressing some of the
problems your original code has, notably, as pointed out by Eric, the
synchronisation issues. You tried to manage those with timers, but
that's hardly ever a good idea.

A couple of points:
1. Don't use tabs in source code. Configure your IDE to convert tabs to
spaces. Four spaces per indent level is the standard.
2. Avoid mixing GUI code and back-end code. Build a model, and then use
that model in the GUI code.
3. *ENCAPSULATE*! In your code, you shared the underlying objects (the
socket, the writer), then built structures on top of them (the
Runnables), and tried to orchestrate those structures via manipulation
of the underlying objects. That's the wrong way to go. Rather, you
should define layers of complexity: the bottom-most, then the first,
second, third, etc., layer. A piece of layer N should only ever interact
with other pieces of layer N, or with pieces of layer N-1, but never
further than that -- never with N-2. It's N-1's responsibility to
interact with N-2. I don't know if that's clear enough, but it's the
best explanation I can come up for now. And it's very important if you
want to build clean software.
4. When catching an InterruptedException, always, always call
Thread.currentThread().interrupt() in the catch block.
5. As a side-note, whenever transmitting text, beware of charset
encodings. I've highlighted this in the code example by specify the
UTF-8 charset. Unless you have a server that can't talk UTF-8, this is
what you should always do.

Code:
-------------------------------------------------------------------
package scratch;

import java.io.BufferedWriter;
import java.io.Closeable;
import java.io.IOException;
import java.iutputStreamWriter;
import java.io.Writer;
import java.net.InetSocketAddress;
import java.net.Socket;
import java.nio.channels.SocketChannel;

public class ConnectionsExample {

public static interface Connector
extends Closeable
{
Writer getWriter() throws IOException;

void reset();
}

public static class Poller
implements Runnable
{
private final Connector
connector
;
private final long
sleepTime
;

public Poller(Connector connector, long sleepTime) {
this.connector = connector;
this.sleepTime = sleepTime;
}

@Override
public void run() {
System.err.println("Entering poller");

for( ; ! Thread.currentThread().isInterrupted(); ){
try {
Writer w = connector.getWriter();

w.write("TEST DATA\n");
w.flush();
}
catch ( IOException ioex ){
System.err.println("Polling failed: ");
ioex.printStackTrace();
connector.reset();
}

try {
Thread.sleep(sleepTime);
}
catch ( InterruptedException iex ){
Thread.currentThread().interrupt();
}
}

System.err.println("Exiting poller");

try {
connector.close();
}
catch ( IOException ioex ){
System.err.println("Could not close connector");
ioex.printStackTrace();
}
}
}

public static Connector createConnector( final String address, final
int port ){
return new Connector(){
private Socket socket;
private Writer writer;
private boolean disposed = false;

@Override
public synchronized Writer getWriter()
throws IOException
{
if( disposed ){
throw new IOException( "Connector is closed" );
}

if( socket != null && ! socket.isClosed() ){
assert writer != null;
return writer;
}
else if( ! socket.isClosed() ){
try {
socket.close();
}
catch ( IOException ioex ){
System.err.println("Could not close old socket:");
ioex.printStackTrace();
}
}

SocketChannel sc;
try {
System.err.printf( "Opening connection to: %s:%d%n",
address, port );
sc = SocketChannel.open(
new InetSocketAddress(address, port)
);
}
catch ( IOException ioex ){
throw new IOException(
String.format("Could not open connection to
%s:%d. Will retry later.", address, port),
ioex
);
}

this.socket = sc.socket();
this.socket.setSoLinger(true, 1);
this.socket.setTcpNoDelay(true);
this.writer = new BufferedWriter(
new
OutputStreamWriter(this.socket.getOutputStream(), "UTF-8") //XXX mind
the charset!!
);

return this.writer;
}

private synchronized void closeSocket( boolean dispose )
throws IOException
{
try {
if( socket != null ){
socket.close();
}
}
finally {
if( dispose ) this.disposed = true;
}
}

@Override
public void reset() {
try {
closeSocket( false );
}
catch ( IOException ioex ){
System.err.println("Could not reset connector:");
ioex.printStackTrace();
}
}

@Override
public void close() throws IOException {
closeSocket( true );
}
};
}

public static Runnable createPoller( String address, int port, long
pollDelay ){
Connector c = createConnector( address, port );
return new Poller( c, pollDelay );
}

public static void createAndStartPoller( String address, int port,
long pollDelay ){
Runnable poller = createPoller( address, port, pollDelay );
Thread t = new Thread( poller );
t.setDaemon( true );

t.start();
}
}
-------------------------------------------------------------------

 

[artik]

Thank You very much! Honestly.
I started analyze Your code but it is partly to hard for me - I'm newbe in Java and it gives me error on invoking method "getWriter" in line:
Writer w = connector.getWriter()
and further exactly inside this method in line "else if( ! socket.isClosed() )". I'm going fight with this in next hours, and don't give up.

One hour ago I write my code as simple as I can and in one thread - everything to avoid problem with synchronization. Effect on the server is still the same (several connection attempts after starting server when client was waiting for it)
I apologize that I'm putting my code again (I hope in good format) instead try to adapt new one from You immediately, but I want to understand this problem.

thrd3 = new Thread(new Runnable() {
@Override
public void run() {
Socket sock = null;
BufferedWriter out = null;
while (true) {
//time to rest for system
try {
Thread.sleep(1000);
} catch (InterruptedException e2) {
Thread.currentThread().interrupt();
}
//attempt to send data
if (sock != null) {
try {
out.write("TEST DATA\n");
out.flush();
} catch (IOException e) {
//catch the error
// ERROR-----------------------
try {
out.close();
sock.close();
sock = null;
} catch (IOException e1) {
sock = null;
out = null;
}
}
} else
// if socket is null try to connect again (or firs time)
{
try {
sock = new Socket();
sock.connect(new InetSocketAddress(address,
5000), 1000);
out = new BufferedWriter(
new OutputStreamWriter(sock
.getOutputStream()));
} catch (IOException e1) {
sock = null;
}
}
}
}
});
thrd3.start();


Best regards,
Artik

 

[Daniele Futtorovic]

Thank You very much! Honestly.
I started analyze Your code but it is partly to hard for me - I'm newbe in Java and it gives me error on invoking method "getWriter" in line:
Writer w = connector.getWriter()
and further exactly inside this method in line "else if( ! socket.isClosed() )". I'm going fight with this in next hours, and don't give up.

One hour ago I write my code as simple as I can and in one thread - everything to avoid problem with synchronization. Effect on the server is still the same (several connection attempts after starting server when client was waiting for it)
I apologize that I'm putting my code again (I hope in good format) instead try to adapt new one from You immediately, but I want to understand this problem.

<snip />

Ah, shoot. I forgot the null check in the line you mention (btw, when
you get an error, always say *what* error it is. Best is to post the
stacktrace of the error; in this case it's obvious, but it's seldom that
easy).

Yes, the format is better this time. Also, you probably have solved your
synchronisation issues for the time being.

After the initial sleep, you should not go into the rest of the loop if
you have been interrupted (being interrupted means "stop doing things",
remember). Likewise, your last code is a regression from before, where
you had the interruptedness as the loop condition (in the while clause),
which is definitely right. In the latest form, AFAICS, the thread won't
exit even if it is interrupted.

As for the problem you mention: it is normal that you'd get several
connection attempts. Do you think there's anything wrong with that?
Client tries, server ain't there, client retries later. Seems okay to
me. What you said before was that your app was crashing after a while in
that case. I'm not sure why that is the case, but a candidate would be
to close() the socket you've created when it fails to connect. I.e.:

try {
sock = new Socket();
sock.connect(new InetSocketAddress(address, 5000), 1000);
out = new BufferedWriter( new
OutputStreamWriter(sock.getOutputStream()));
} catch (IOException e1) {
if( sock != null ){
try { sock.close(); } catch( IOException ioex ){
ioex.printStacktrace(); }
sock = null;
}
}

 

[artik]

Thank you again for your advising - it is worth for me so much.
When we back to your code and checking null, is my "improvement" your code good:
@Override
public synchronized Writer getWriter()
throws IOException
{
if( disposed ){
throw new IOException( "Connector is closed" );
}

if( socket != null && ! socket.isClosed() ){
assert writer != null;
return writer;
}
else if( socket != null ){

It's working but sometimes new null error happens in different place (some lines below - in my project it is 125th row of your code):

this.socket = sc.socket();

Here you are description:
12-09 22:06:52.982: E/AndroidRuntime(1387): FATAL EXCEPTION: Thread-9
12-09 22:06:52.982: E/AndroidRuntime(1387): java.lang.NullPointerException
12-09 22:06:52.982: E/AndroidRuntime(1387): at com.example.aj.siec.ConnectionsExample$1.getWriter(ConnectionsExample.java:125)
12-09 22:06:52.982: E/AndroidRuntime(1387): at com.example.aj.siec.ConnectionsExample$Poller.run(ConnectionsExample.java:50)
12-09 22:06:52.982: E/AndroidRuntime(1387): at java.lang.Thread.run(Thread..java:1019)


Beside this your proposition of code is almost perfect! Almost, because I can't reduce time of reconnection (like in my simple code using timeout for breaking attempt of connection). After long time waiting for server, after I turn on it - only one attempt (not like in my code over a dozen ) is taking but time (from the moment I turn on server) for waiting on connection takes about 50-70seconds with only waiting for set connection.

I think my code beside point what you were show, is bad because I think it have works in schema:
1. server is switched off
2. client tries to connect to server but attempt fails
3. client closes socket and try again point.2 until pass to connect
4. only last (succesfull) attempt connects to server is initializing rest of connection.

But in my "sad" example - it looks like all attempts from point are remember and when point 3 happend they try to connect again and immediately finishafter connection - for the rest of the time only one of them (I mean last and (pass attempt) continues connection. Beside problem of strange attempt for connect after server woke up is that it is crush my application still -but the time is needed some longer than before.

Regards,
Artik

 

[Daniele Futtorovic]

Thank you again for your advising - it is worth for me so much. When
we back to your code and checking null, is my "improvement" your code
good: @Override public synchronized Writer getWriter() throws
IOException { if( disposed ){ throw new IOException( "Connector is
closed" ); }

if( socket != null && ! socket.isClosed() ){ assert writer != null;
return writer; } else if( socket != null ){

It's working but sometimes new null error happens in different
place (some lines below - in my project it is 125th row of your
code):

this.socket = sc.socket();

Here you are description: 12-09 22:06:52.982: E/AndroidRuntime(1387):
FATAL EXCEPTION: Thread-9 12-09 22:06:52.982: E/AndroidRuntime(1387):
java.lang.NullPointerException 12-09 22:06:52.982:
E/AndroidRuntime(1387): at
com.example.aj.siec.ConnectionsExample$1.getWriter(ConnectionsExample.java:125)

12-09 22:06:52.982: E/AndroidRuntime(1387): at
com.example.aj.siec.ConnectionsExample$Poller.run(ConnectionsExample.java:50)

12-09 22:06:52.982: E/AndroidRuntime(1387): at
java.lang.Thread.run(Thread..java:1019)


Beside this your proposition of code is almost perfect! Almost,
because I can't reduce time of reconnection (like in my simple code
using timeout for breaking attempt of connection). After long time
waiting for server, after I turn on it - only one attempt (not like
in my code over a dozen ) is taking but time (from the moment I turn
on server) for waiting on connection takes about 50-70seconds with
only waiting for set connection.

I think my code beside point what you were show, is bad because I
think it have works in schema: 1. server is switched off 2. client
tries to connect to server but attempt fails 3. client closes socket
and try again point.2 until pass to connect 4. only last (succesfull)
attempt connects to server is initializing rest of connection.

But in my "sad" example - it looks like all attempts from point are
remember and when point 3 happend they try to connect again and
immediately finish after connection - for the rest of the time only
one of them (I mean last and (pass attempt) continues connection.
Beside problem of strange attempt for connect after server woke up is
that it is crush my application still - but the time is needed some
longer than before.

Artik,

Firstly, generally speaking, it is indeed important to have good code,
but it is at least just as important to have *code you understand*.
Because if you don't understand your code, then you won't be able to
debug or improve it, which is almost always a requirement.

The bits of code I offered you were mainly intended to give you (however
efficient) hints as to how to structure a program. That is, that you
should analyse (this is an abstract, mental exercise) what the processes
and actors are in the "things the program does", to regroup what belongs
together and separate what doesn't, and to then represent these using
classes. I don't know if that makes much sense; I reckon it doesn't.
What I want to say is that if you feel it's going over your head, you'd
probably be best advised to leave it aside for study when you have the
time, and stick to something you feel comfortable with.

Secondly, yes, the correction you made should be okay.

Thirdly, the NullPointerException NPE you mention seems really weird. I
don't see how that could happen if you've used the same code.

Fourth, I've used the SocketChannel method because I find it a bit
cleaner and because it's part of the NIO package, which is overall an
improvement over the older IO package. But indeed, a problem with that
is you can't set the socket timeout using that method, and your original
code was doing that, so it was actually a mistake to use it. If you want
to specify a connection timeout (what you called "time of
reconnection"), you should stick to the code you used previously (i.e.,
creating a new Socket() and then connect()ing it).

Lastly, I must admit I have trouble understanding exactly what you are
saying, but there's another point I'd like to address, rather. That is,
how often are you creating those polling/connection objects or threads?
Your original code seemed to do it on the press of a UI button. Are you
taking care of cleaning up the previous state if the button is pressed a
second time?

 

[artik]

try {

sock = new Socket();

sock.connect(new InetSocketAddress(address, 5000), 1000);

out = new BufferedWriter( new

OutputStreamWriter(sock.getOutputStream()));

} catch (IOException e1) {

if( sock != null ){

try { sock.close(); } catch( IOException ioex ){

ioex.printStacktrace(); }

sock = null;

}

}

I don't know why yesterday - No, but today Yes - it works perfect for me.
Thank You again
Artik

 

[Lew]

Umm, no. This part belongs in a 'finally' block, not a 'catch' block. Oops.

You need to format your code better. This is virtually unreadable.

With a 'final sock' variable and multiple try-catch blocks you can avoid the check for 'null'
and the reset to 'null'. (You probably don't need the reset now anyway.)

I don't know why yesterday - No, but today Yes - it works perfect for me.

I doubt that very much. As written it is buggy.