java - Thread for reading from socket stream takes more CPU usage -
in client socket, wrote thread read socket's inputstream continuously. here have used while loop read infinitely. takes more cpu; hence possible reduce cpu. please add suggestions. possible add listeners inputstream.
thread code:
public void run() { while (!shutdown) { try { if(socketclient != null) { string message = socketclient.getmessage(); logger.info ("message size:" + message.length ()); if(!message.equals("emptystring")) { process(message); } } } catch (exception exception) { logger.info("unable read socket message" +exception); } }
}
socketclient.java
public class socketclient{ private volatile boolean isconnected; private int port; private int retrycount; private long starttime; private string hostname; private datainputstream input; private dataoutputstream output; private socket socket; public socketclient(int port, string hostname) throws ioexception { this.port = port; this.hostname = hostname; establishconnection(); } public void shutdown() { try { shutdown = true; input.close(); output.close(); socket.close(); } catch (exception e) { logger.debug("exception in shutdown:" + e.getmessage()); } } public string getmessage() { bufferedreader reader = null; try { stringbuilder builder = new stringbuilder(); reader = new bufferedreader(new inputstreamreader(tcpsocket.getinputstream())); { builder.append(reader.readline()); } while((reader.ready())); if (builder.length() == 0) return "emptystring"; return builder.tostring(); } catch (ioexception e) { return "emptystring"; } { try { if(reader != null) reader.close(); } catch(ioexception e) { logger.error("unable close reader"); } } } private void establishconnection() { retrycount = 1; starttime = system.currenttimemillis(); while (!shutdown) { try { if(!isconnected) { socket = new socket(hostname,port); socket.setkeepalive(true); input = new datainputstream(socket.getinputstream()); output = new dataoutputstream(socket.getoutputstream()); isconnected = true; shutdown = true; } } catch (exception exception) { isconnected = false; sleepfewseconds(); reconnectsocket(); } } } private void reconnectsocket() { long endtime = starttime + 120000l; if(!(system.currenttimemillis() < endtime)) { shutdown = true; } } private void sleepfewseconds() { try { timeunit.milliseconds.sleep(20); } catch (interruptedexception interruptedexception) { shutdown = true; } } }
i going critique entire class here. answer specific question appear.
public class socketclient{ private volatile boolean isconnected;
you don't need this. socket == null
well.
private int port; private int retrycount; private long starttime; private string hostname; private datainputstream input; private dataoutputstream output; private socket socket; public socketclient(int port, string hostname) throws ioexception { this.port = port; this.hostname = hostname; establishconnection(); } public void shutdown() { try { shutdown = true; input.close(); output.close(); socket.close();
you don't need these closes, , you're doing them in wrong order anyway. output.close()
sufficient , in case should first.
} catch (exception e) { logger.debug("exception in shutdown:" + e.getmessage()); } } public string getmessage() { bufferedreader reader = null;
the bufferedreader
should instance variable, not local variable. it's buffered. if make local variable lose data.
try { stringbuilder builder = new stringbuilder(); reader = new bufferedreader(new inputstreamreader(tcpsocket.getinputstream())); { builder.append(reader.readline()); } while((reader.ready()));
you don't need this. if message single line, need return reader.readline()
, , need caller check whether null, , if close socket, cease reading, etc. if message more 1 line, misuse of ready()
: not indicator of end of message. appears comments under question shouldn't have method: connect socket input stream directly xml parser , let it reading.
if (builder.length() == 0) return "emptystring";
don't this. return ""
or null. don't make new magic strings application have decode.
return builder.tostring(); } catch (ioexception e) { return "emptystring";
ditto.
} { try { if(reader != null) reader.close();
you should not close reader here. closing close socket, can never message.
} catch(ioexception e) { logger.error("unable close reader"); } } } private void establishconnection() { retrycount = 1; starttime = system.currenttimemillis(); while (!shutdown) { try { if(!isconnected) { socket = new socket(hostname,port); socket.setkeepalive(true); input = new datainputstream(socket.getinputstream()); output = new dataoutputstream(socket.getoutputstream()); isconnected = true; shutdown = true;
why setting shutdown
true
here? nothing shutdown yet. it's brand new socket.
} } catch (exception exception) { isconnected = false; sleepfewseconds(); reconnectsocket(); }
poor practice. socket.connect()
, called internally new socket(...)
, retries, , should distinguish between connection-failure exceptions rather adopt same strategy them all. example, 'connection timeout' have blocked minute or so: don't need another sleep; , 'connection refused' means there nothing listening, retrying pointless.
private void reconnectsocket() { long endtime = starttime + 120000l; if(!(system.currenttimemillis() < endtime)) { shutdown = true; } } private void sleepfewseconds() { try { timeunit.milliseconds.sleep(20);
this not 'few seconds'. 20 milliseconds, , not enough @ least 2 orders of magnite in network programming, extent there should sleep @ of course.
} catch (interruptedexception interruptedexception) { shutdown = true;
shutdown
appears never false. doubt you've thought through means, , doubt need @ all.
as calling code:
public void run() { while (!shutdown) { try { if(socketclient != null) {
if socketclient
null loop spin meaninglessly. surely method should construct socket client?
string message = socketclient.getmessage(); logger.info ("message size:" + message.length ());
here failing check null , failing respond appropriately, close socket , exit loop. instead npe here.
if(!message.equals("emptystring")) { process(message);
see above. don't send special text messages. happens if peer needs send 1 day?
} } } catch (exception exception) { logger.info("unable read socket message" +exception);
unacceptable. catch inside loop , ignores exception. result that, again, loop spin meaninglessly on exception. , methods you're calling should declared throw ioexception
, , should catch here. @ present spin on nullpointerexception
.
Comments
Post a Comment