Hm. There is a lot of very dubious stuff going on in your code. At a guess, I think you are not using strict, and only adding it to avoid critism when posting. This is a false economy--actually it's no economy at all. Please do not be upset by what follows, it is intended to help you, and to reduce the confusion in your code to the point where the problem you are having becomes soluable.

To that dubious stuff:

1. Why are you using our instead of my.

   There are very specific times when our is useful, but unless you know that you need it, you probably don't and should not use it. So, switch all those ours to mys.

2. Why are you declaring variables miles away from there point of use? Eg. $k $n $m $p.

   One of those isn't even used ($m), and the rest are used a loop counters. $p is declared in the main thread and used in the thread procedure.

   Switch them to my and declare them at their point of use:

   for( my $p = 0; ...)
   [download]

   I might come back and deal with the C-style for loops later.

   Likewise, $dup, $SyncHeader, $SyncID, $UnitID, $sel_cli2 can all be declared at the point of use; and $buf2 & $ref can deleted entirely.

3. Threads get copies of non-shared variables that exist when they are spawned!

   So, the copy of my @Mdevices; that the main thread has will never see changes made by the thread procedure. That seems to be a part of your mechanism for cleaning up socket handles, and it would have to be shared for that to stand a chance of working. But things are still confused enough that I'm not sure it would work even if it was shared. So I'll come back to that.

4. Your thread proc (reformatted a litte to prevent wrapping) contains lots of problems:

   Please see the embedded comments!

   sub processit {
       my( $lclient, $lfileno, $lpeer ) = @_;
   
       my $sel_cli_id = $num_of_client;  ######### This is never used.
   
       if( $lclient->connected ){
           while( <$lclient> ) {
   
               ### $peerC si shared by cloning
               ### It sole purpose appears to be to allow you to share 
               ### a single thread proc by two different types of thread.
               if( $peerC eq '127.0.0.1' ) { 
                   my(@comm) = split(/:/, $_);  ### These 3 lines is bett
   +er as one
                   my $sel_clix = $comm[0];
                   my $commandC = $comm[1];
   
                   my $sel_cli2;
   
                   ### This will only see a copy of whatever is in @MDevi
   +ces 
                   ### when this thread is spawned. 
                   ### And none of teh subsequent changes!
                   ### You'd be far better avoiding a linear search by us
   +ing a hash
                   for( my $p=0; $p <= $#Mdevices; $p++ ) {
                       if( $Mdevices[ $p ][ 0 ] eq $sel_clix ) {
                           $sel_cli2 = $Mdevices[ $p ][ 1 ];
                       }
                   }
                   ### A loop that iterates a single thing??
                   foreach my $fn ( $sel_cli2 ) {
                       if( $sel_cli2 ) {
                           open my $fh, ">&=$fn" or warn $! and die;
                           print "\n $lclient $sel_cli2 Command ",
                             "sent to: $sel_clix Command: $commandC";
                           print $fh  "$commandC\n"
                       } else {
                           print "\n $lclient Device: $sel_clix",
                            "NOT avaiable to receive command\n";
                       }
                   }
               }
               else {
                   print "COMMAND RESPONCE: $_";
               }
           }
       }
       close( $lclient );
       #### Modifying a shared variable without locking it 
       #### Will cause problems.
       @clients = grep{ $_ !~ $lfileno } @clients;
   }
   [download]

   What makes th above code so confusing is that you are trying to use one thread procedure to do double service for two different types of thread: The gprs clients and the localhost command entry client.

   It would be far better to have two thread procedures and start the appropriate one depending upon where the in-bound connection comes from. Each of the thread procs would be simpler and need to share less information.

5. The main thread.

   Most of the complexity of your main thread comes from using an array @MDevices, and having to iterate it (twice!) to first find, and then delete things, when if you used a hash that could be done with one or two single statements.

The problem you cite: "And there is my problem: Old threads remains connected to the server and I need to get rid of them!" is caused by having multiple handles (one in the main thread and another in the client threads),connected to each client, but only closing one of them.

You are (I think) attempting to deal with this using @MDevices, but as it is not shared, the changes are not being propogated.

This post is just by way of explanation of what is wrong. It'll take a while to work out (and test given I don't have any gprs devices) a solution, but I'll try to post something later today or tomorrow.

Hello BrowserUk

Thanks for your help, I'm not upset at all, actually I'm very happy for your help.
About strict, it was really used, I would never put strict just to avoid criticism.

Can't wait to see your next post.

Best regards
Igor

A few questions:

1. You say above that Old threads remains connected to the server and I need to get rid of them!

   Do you mean: old threads within the server persist (continue to run)? For a while? Forever? Or

   Do you mean old connections persist? For a while? Forever?

   Either way, how long have you waited to see if they (whichever) will clean themselves up?

2. Does your current code work? Ie:
3. Do your commands get sent to the gprs devices?
4. Does that happen immediately, or with some delay?
5. From a previous thread, I recall that these gprs devices regularly send data (GPS coordinates?) to the server. What frequency does that happen?

This code should be essentially similar to your existing code (and may even display the same problems; though I'm hoping not).

Give it a try and let me know how you get on. (Error handling is perfunctory!).

#! perl -slw
use strict;
use IO::Socket;
use threads;
use threads::shared;

$|++;
print "$$ Server started\n";

my %devices :shared;

my $server = new IO::Socket::INET(
    Timeout   => 500,
    Proto     => "tcp",
    LocalPort => 7777,
    Reuse     => 1,
    Listen    => 5
);

while( my $client = $server->accept ) {
    next unless defined $client;

    my $peerhost = $client->peerhost;

    read $client, my $packet, 8
        or warn "Couldn't read sync packet: $!"
        and close $client and next;

    ## No use for syncHeader or syncID so discard them
    my $unitId = unpack 'x[vv]V', $packet;

    ## start the appropriate type of client thread
    threads->create(
        $peerhost == '127.0.0.1' ?  \&cmdClient : \&gprsClient,
        $unitId == 437918234 ? \&cmdClient : \&gprsClient,
        $client, $unitId
    )->detach;
}

sub cmdClient {
    my( $client, $unitId ) = @_;

    my $fileno = fileno $client;
        warn "Command Client running on $fileno\n";

    while( <$client> ) {
        my( $unitId, $cmd ) = split ':';
        warn "Got command '$cmd' for [$unitId]\n";

        ## Get the fileno for the required unitId
        my $gprsFno = do{
             lock %devices;
             unless( exists $devices{ $unitId } 
                and defined $devices{ $unitId } 
             ) {
                 warn "No device with unitId '$unitId' currently conne
+cted" 
                 and next
             };
             $devices{ $unitId }
        };

        ## dup it for output (NOTE: '>&' NOT '>=&')
        open my $fh, '>&' . $gprsFno
            or warn "Failed to dup( $gprsFno ) [$unitId]: $!"
            and next;

        ## Send the command (Won't happen (on my system) 
        ## until that unit sends us something!!!)
        print $fh $cmd
            or warn "Failed to write command '$cmd' to [$unitId]"
            and close $fh and next;

        warn "Sent '$cmd' to [$unitId]";
        close $fh;
    }
    close $client;
    warn "cmdClient [$unitId] disconnected\n";
}

sub gprsClient {
    my( $client, $unitId ) = @_;

    my $fileno = fileno $client;

    ## Add device to shared lookup table indexed by $unitId
    { lock %devices; $devices{ $unitId } = $fileno; }
    warn "gprsClient [$unitId] running on $fileno\n";

    ## Simply echo all input from the gprs device
    print "$unitId: Command response: $_" while <$client>;

    ## When the connection terminates, remove it from the lookup
    lock %devices; delete $devices{ $unitId };
    close $client;
    warn "gprsClient [$unitId] disconnected\n";
}
[download]

---

Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.

"Science is about questioning the status quo. Questioning authority".

In the absence of evidence, opinion is indistinguishable from prejudice.

"Too many [] have been sedated by an oppressive environment of political correctness and risk aversion."

#close filehandle before detached thread dies out
print "closing lclient $lclient\n";
    close( $lclient);
    #remove multi-echo-clients from echo list
    @clients = grep {$_ !~ $lfileno} @clients;
print "exiting thread .....clients = @clients\n";
[download]

The clients are always connected to the server, except when is some network outage or other gprs disconnections. Then, clients reconnects to the server And there is my problem: Old threads remains connected to the server and I need to get rid of them!

An interesting problem, but it seems that the disconnect should be detected. I wish I had a test setup with many GPS devices to test with.

The first thing that comes to mind, is to keep a hash of connected GPS devices, tracking their id and fileno's, and check for duplicate connections in your server->accept. Another thing is maybe try a different test in your thread for being connected. The connected method returns the address if connected, so maybe you can track addresses, and be sure there are no duplicate addresses?

Maybe try a shorter timeout value? A timeout of 500 is a long time, and may be allowing your device to reconnect before the thread detects the old connection is gone?

Hello Zentara,

I've tried with debugging statements and you were right, the control never gets to the end.
I've tried with shorter timeout, but there is no difference.

If you or BrowserUk, need, I can route my gprs device to some other IP address and simulate disconnects

Regards, Igor

If you or BrowserUk, need, I can route my gprs device to some other IP address

No thanks....sounds like work. :-)

You seem to be isolating the problem, what you need to do now is simplify your example for testing/debugging. Always try to start with the simplest setup, get it working, then add complexity. In your case, just try testing 2 GPS devices, and simplify your thread code (or even remove it and just go with IO::Select), until you see disconnects. Try removing all those If statements, and just have super simple code, like:

 if($lclient->connected){
           print "1\n";   
         } else{
         print "not connected\n"
   }
[download]

Then start building from there. You should be able to turn off your device, and see what happens. Play with simple code until it's working under your control.

I have a feeling that your GPS devices may not be acting like a regular computer would, and you may need to put some IO::Select code into your thread code, so you can use it's can_read, can_write, and has_exception methods to detect problems in the GPS device. You can setup IO::Select to run on just the single client in each thread, like this untested pseudocode:

my $sel = new IO::Select(); 
$sel->add($lclient);              
if($lclient->connected){
          print "connected\n";   
          print "can read\n"  if  ($sel->can_read);
          print "can write\n"  if  ($sel->can_write);
          print "has exception\n"  if  ($sel->has_exception);
} else{
    print "not connected\n"
}
[download]

---

I'm not really a human, but I play one on earth Remember How Lucky You Are

If you want a thread to do something you need a means to communicate with it. In this case (AFAICS -- I regret I did not have the patience to wade through the code you posted, partly because it was all over the place) you want the thread(s) either to give up or to reestablish connection.

You could use a shared variable that the thread(s) could check on a regular basis. (You could use a Thread::Queue or a Thread::Semaphore, but that's probably over the top.)

You may be able to use a signal, see thread-signalling, depending on version of perl etc.

I've downvoted your post because nothing in it will in any way assist the OP.

If you want a thread to do something you need a means to communicate with it.

This is completely wrong. When the OP starts his threads, he is passing in all the information they require to function and he never needs to communicate with it again before it dies.

The problem he is having has absolutely nothing to do "communication" (he is already using shared variables), and not only will neither of the modules you mention allow him to fix his problem, they will most likely confuse him further. (And he is already quite confused enough!).

In particular, the link you gave is guarenteed to create confusion. Thread signalling is an almost entirely useless concept, and could never be of any use n fixing the problem the OP has.

---

Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.

"Science is about questioning the status quo. Questioning authority".

In the absence of evidence, opinion is indistinguishable from prejudice.

"Too many [] have been sedated by an oppressive environment of political correctness and risk aversion."

This is completely wrong. When the OP starts his threads, he is passing in all the information they require to function and he never needs to communicate with it again before it dies.

I clearly missed the point :-( I thought the OP problem was how to kill off the threads in the event of losing connection to the server, so that everything could be restarted again. If they were separate processes, then one would expect to kill each process. My understanding is that with threads there isn't an equivalent mechanism. So, that what you need is a way to tell each thread that it's time to terminate themselves -- which requires some form of communication.

I apologise without reservation if (a) I have misunderstood the point; and/or (b) misunderstood thread handling; and/or (c) failed to express what I intended.