Discard Server, threads not cleaning up

[James Sager]

Hello,

I am running the discard server socket echo example.

When I run my Flash client, everything works fine, except the threads don't get cleaned up nicely.

When the Flash Client quits, the Discard Server in Java says:
    at sun.nio.ch.SocketDispatcher.read0(Native Method)
    at sun.nio.ch.SocketDispatcher.read(Unknown Source)
    at sun.nio.ch.IOUtil.readIntoNativeBuffer(Unknown Source)
    at sun.nio.ch.IOUtil.read(Unknown Source)
    at sun.nio.ch.SocketChannelImpl.read(Unknown Source)
    at io.netty.buffer.UnpooledUnsafeDirectByteBuf.setBytes(UnpooledUnsafeDirectByteBuf.java:447)
    at io.netty.buffer.AbstractByteBuf.writeBytes(AbstractByteBuf.java:881)
    at io.netty.channel.socket.nio.NioSocketChannel.doReadBytes(NioSocketChannel.java:225)
    at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:119)
    at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:511)
    at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:468)
    at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:382)
    at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:354)
    at io.netty.util.concurrent.SingleThreadEventExecutor$2.run(SingleThreadEventExecutor.java:116)
    at io.netty.util.concurrent.DefaultThreadFactory$DefaultRunnableDecorator.run(DefaultThreadFactory.java:137)
    at java.lang.Thread.run(Unknown Source)

And threads keep stacking up one for every time I run my client:

Thread [nioEventLoopGroup-2-1] (Running)   
Thread [nioEventLoopGroup-3-1] (Running)   
Thread [nioEventLoopGroup-3-2] (Running)   
Thread [nioEventLoopGroup-3-3] (Running)   
Thread [nioEventLoopGroup-3-4] (Running)   
Thread [nioEventLoopGroup-3-5] (Running)   
Thread [nioEventLoopGroup-3-6] (Running)   
Thread [nioEventLoopGroup-3-7] (Running)   
Thread [nioEventLoopGroup-3-8] (Running)   

etc etc etc

This looks like the sign of a memory leak that will eventually crash the program.

How do I fix it?

Thank you,

James

[이희승 (Trustin Lee)]

The I/O threads start lazily, but they keep running until you tell them to shut down via EventLoopGroup.shutdownGracefully().

 

--

https://twitter.com/trustin

https://twitter.com/trustin_ko

https://twitter.com/netty_project

--

You received this message because you are subscribed to the Google Groups "Netty discussions" group.

To unsubscribe from this group and stop receiving emails from it, send an email to netty+un...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/netty/CADMLN6b0K9Z2Pd%3DKTNapiqUbFwngZ%3D_Pp%3D%2BKCxrpdk0rSSxJMw%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

 

[goodnews...@gmail.com]

How can I call .shutdownGracefully() to clean up some threads without losing connection to active ones?

Is it okay for tens of thousands of inactive threads or more to pile up?

[goodnews...@gmail.com]

Also thank you for the reply.

[Tim Boudreau]

Managing the shutting down of things is something that isn't usually done well in Java - probably since finalize() methods are discouraged, people get the idea that managing the shutdown of anything is a little bit uncouth.  But the fact is that there are things that do need the end of their lifecycle managed - things like thread pools, timers, connections, connection pools and so forth.

I use Guice a lot for dependency injection, and that allows for a handy little abstraction (which you could really do in any sort of code) such as this:

https://github.com/timboudreau/giulius/blob/master/giulius/src/main/java/com/mastfrog/giulius/ShutdownHookRegistry.java

So, anything that starts a timer or thread pool or whatever just adds it (or a Runnable that does cleanup) to the shutdown hook registry.

This ends up having the nice side-effect that you don't have to wait for JVM shutdown to run your shutdown hooks - you can write, for example, a server that cleanly unloads and reloads itself without any downtime, and assuming you aren't abusing static fields, no memory leaks (something that Java EE servers have been trying and failing to do for almost 20 years).

So I'd encourage using some sort of abstraction for "stuff that needs to be shut down" and a trigger for doing that shutdown, rather than trying to handle this sort of thing on a case-by-case basis, which gets unmanageable fast.

-Tim

[Tim Boudreau]

On Friday, May 8, 2015 at 8:54:12 PM UTC-4, goodnews...@gmail.com wrote:

How can I call .shutdownGracefully() to clean up some threads without losing connection to active ones?

Is it okay for tens of thousands of inactive threads or more to pile up?

You pay a price in memory and OS-level resources for every thread, active or not - so, the answer is, no.

But in a Netty-based server, unless you're doing something very weird, you shouldn't need "tens of thousands of threads" in the first place - done right, a relatively small number of threads can handle 10k+ concurrent connections with Netty.  With an asynchronous server technology, unlike with servlets, when you have too few threads, work slows down, it doesn't grind to a halt.

That said, if the code that services requests does blocking I/O (say, JDBC), that can drive up the number of threads you need - but for that case, there is no point in having more threads than the size of your JDBC connection pool.

What you probably want to do is set a hard limit on the number of threads your EventLoopGroup (which is essentially just a fancy ExecutorService) can have, and measure using that and see how many threads you need - and start testing with very low numbers (like 2, or 5) no matter what guess you have about the right number.

In general, never, ever use an unbounded thread pool in production code.

-Tim

[goodnews...@gmail.com]

Tim,

Thank you for confirming my thoughts that many threads is bad.

Should I post my code?
Or
Can someone point me to a scalable echo server?

My goal is simply this:
Accept incoming sockets.
Anytime someone sends data, all connected sockets get it.

I'm working on a custom chat room for starters.
Next up is a collectable card game.
Anyone who wants to alpha/beta a space based ccg, leave info here too.

[Tim Boudreau]

I'd do something like this, and keep one instance of it around.  "Listener" connections get their Channel passed to register, and they'll all be fed anything passed to send().

public class Channels {

    private final Set<Channel> registered = Sets.newConcurrentHashSet();

    /**

     * Call this from clients that connect and listen.

     *

     * @param channel The channel, which you do not close

     */

    public void register(Channel channel) {

        channel.closeFuture().add(new ChannelFutureListener() {

            @Override

            public void operationComplete(ChannelFuture f) throws Exception {

                registered.remove(f.channel());

            }

        });

    }

    /**

     * Call this from clients that want to send some data to all listeners.

     *

     * @param buf The data to send

     */

    public void send(ByteBuf buf) {

        for (Channel channel : registered) {

            ByteBuf toSend = buf.duplicate();

            channel.writeAndFlush(toSend);

        }

    }

}

That's about the simplest possible implementation.  Since this will block until all clients have been sent the bytes, it might be cleaner to break this work up into multiple rounds in the EventLoopGroup, one per client, so even with a 1-2 of threads, your server can service new inbound requests while sending the previous data happens in the background.  That would look something like this - you just have a runnable that re-schedules itself until its Iterator<Channel> is empty.

public class Channels {

    private final Set<Channel> registered = Sets.newConcurrentHashSet();

    private final EventLoopGroup threadPool;

    public Channels(EventLoopGroup group) {

        this.threadPool = group;

    }

    /**

     * Call this from clients that connect and listen.

     *

     * @param channel The channel, which you do not close

     */

    public void register(Channel channel) {

        channel.closeFuture().add(new ChannelFutureListener() {

            @Override

            public void operationComplete(ChannelFuture f) throws Exception {

                registered.remove(f.channel());

            }

        });

    }

    /**

     * Call this from clients that want to send some data to all listeners.

     *

     * @param buf The data to send

     */

    public void send(ByteBuf buf) {

        threadPool.submit(new Sender(buf.duplicate()));

    }

    private class Sender implements Callable<Void> {

        private final Iterator<Channel> remainingChannels;

        private final ByteBuf data;

        Sender(ByteBuf data) {

            this.data = data;

            remainingChannels = registered.iterator();

        }

        @Override

        public Void call() throws Exception {

            try {

                if (remainingChannels.hasNext()) {

                    Channel channel = remainingChannels.next();

                    channel.writeAndFlush(data);

                }

            } finally {

                if (remainingChannels.hasNext()) {

                    threadPool.submit(this);

                }

            }

            return null;

        }

    }

}

-Tim

--
You received this message because you are subscribed to a topic in the Google Groups "Netty discussions" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/netty/RZRtsXCeMHQ/unsubscribe.
To unsubscribe from this group and all its topics, send an email to netty+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/netty/4742d583-323c-4a1b-b4cb-bbaa44838869%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

http://timboudreau.com

[Tim Boudreau]

On Monday, May 18, 2015 at 11:32:28 PM UTC-4, goodnews...@gmail.com wrote:

Tim,

Thank you for confirming my thoughts that many threads is bad.

Should I post my code?

Sure.  If you're winding up with thousands of threads, I'm kind of curious what you're doing.  Any chance you're creating a new thread pool for every request or something?

 

Can someone point me to a scalable echo server?

I think you could take the code I posted and create one pretty simply.

-Tim

[goodnews...@gmail.com]

Here is my code:
DiscardServer.java
http://pastebin.com/5GxnqKb3
DiscardServerHandler.java
http://pastebin.com/PH1gyNmu

The thing with me is that I'm a quality coder, but I'm 100% a Netty noob.
I want to be able to read sockets coming in, and to write to sockets different data.  In this example I posted, its a weak chat server.  Anything one socket writes, echos to everyone.

The problem with the code I'm using is two fold:
1) It makes a new thread with every socket connection
2) If the same user reconnects multiple times, they get multiple echos for every time they've connected.

Any help is appreciated.   Thank you for your help Tim. 

[Tim Boudreau]

Okay, I think you can solve the thread creation problem by passing a thread count to your EventLoopGroup constructors.  Try this to start, and increase the number only when you've proven you need to:

        EventLoopGroup bossGroup = new NioEventLoopGroup(1);

        EventLoopGroup workerGroup = new NioEventLoopGroup(2);

(yes, that's crazy conservative, and yes, that's where you should start).

Now, about the rest of the code:  wildloop() does not do what you think it does, and will have all sorts of problems:  

1.  There's no guarantee that you will process a message from a client before the next one comes and clobbers the previous one - you're going to lose data.  

2.  For another thing, the field "stringbuffer" is not marked volatile, and there's nothing to make it thread-safe, but it's being written on one thread and read on another, and there's nothing to stop it being overwritten before it has been received.  That is not a workable way to pass messages between threads (see below for something that will work).

3.  You're keeping a pool of DiscardServerHandler instances (and really you should have exactly one), but there's no guarantee I can see that you won't use one that's already been used by a different client, and wind up reading a message from some other client as coming from the one it's now used with

Here's a rough sketch of how this should work:

 - Create your DiscardServerHandler in its field definition - there will be exactly one (I think you need to annotate it with @Shareable?).  Use the same handler for all requests.  And don't have any state - any fields you set when processing a message in that class.

 - If you can, process messages from clients where they are, as they come in - don't have a separate loop at all.  To distribute data to other clients, do something like the code I posted earlier;  probably using the second technique.

 - If you can't process messages from clients as they come in, do have a separate loop, on a separate thread (or the main thread, whatever).  But use a thread-safe queue - have the handler throw the message into the queue, and have that loop just loop polling queue.take() and sending on whatever it has.  ConcurrentLinkedQueue would be a good starting place - http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ConcurrentLinkedQueue.html

The main thing to keep in mind writing something like this is, anything that is going to be accessed by more than one thread needs special treatment - like using a Queue.  Things like the JDK's *Queue classes are very useful because they take care of a lot of the hard parts of that for you.  In particular, since not all tutorials mention it - if you're going to set a field on object X from thread A, and read that field it from thread B, you must either mark it as volatile or use synchronization, unless it is a final field set when the object is being constructed.

Take a look at some concurrency tutorials - they will help:

http://docs.oracle.com/javase/tutorial/essential/concurrency/

Best of luck,

-Tim

--
You received this message because you are subscribed to a topic in the Google Groups "Netty discussions" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/netty/RZRtsXCeMHQ/unsubscribe.
To unsubscribe from this group and all its topics, send an email to netty+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/netty/c096c26d-01f0-4207-a91b-9cae7cf65b9a%40googlegroups.com.

[goodnews...@gmail.com]

Thank you a ton, Tim!

I'm moving full speed on a new social media market. If it flies, it could be good. I think I can get this social media site done in 1-3 months.

I'll keep you guys in the loop with my finished netty projects as they happen.

[goodnews...@gmail.com]

My social media app is done. It just suffers from a chicken and egg problem. No users so no sense putting more data in. I figured as much when I started. I'm just glad I have another project done. I am going to work on a collectable card game next: Starfighter General. This one should take 3-6 months.

[Tim Boudreau]

That's always the problem - writing something that 2 users will want to use when there are only 2 users :-)  There's a similar problem in evolution:  Those few accidentally photosensitive cells have to provide survival value to the organism, or they'll never evolve into an eye.

-Tim

On Fri, Jun 19, 2015 at 1:04 AM, <goodnews...@gmail.com> wrote:

My social media app is done.  It just suffers from a chicken and egg problem.   No users so no sense putting more data in.  I figured as much when I started.  I'm just glad I have another project done.  I am going to work on a collectable card game next: Starfighter General.  This one should take 3-6 months.

--
You received this message because you are subscribed to a topic in the Google Groups "Netty discussions" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/netty/RZRtsXCeMHQ/unsubscribe.
To unsubscribe from this group and all its topics, send an email to netty+un...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/netty/3d74cfa9-a02d-4cf6-a818-9695020b5f55%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

http://timboudreau.com