Codigo en Java que no hace lo que deberia

[xoker]

Buenas, a ver si me podeis explicar que ocurre con este codigo:

Resulta que deberia de darme un resultado final de cero, pero en vez de eso me da un resultado indeterminado, os pongo aqui el codigo:

Código [Seleccionar] Expandir

public class Hilo extends Thread {
    private int tipoHilo;
    private  static Integer n = new Integer(0);
    private int nVueltas;

    public Hilo(int nVueltas, int tipoHilo)
    {this.nVueltas=nVueltas; this.tipoHilo=tipoHilo;}

    public void run()
    {
     
      switch(tipoHilo){
        case 0:
        for(int i=0; i<nVueltas; i++)
        synchronized(n){ n=n+1;};
        break;
        case 1:
        for(int i=0; i<nVueltas; i++){
        synchronized(n){n=n-1;};
        break;
      }
    }

  public static void main(String[] args)
      throws Exception
  {
      Hilo p = new Hilo(10000, 0);
      Hilo q = new Hilo(10000, 1);
      p.start();
      q.start();
      p.join();
      q.join();
      System.out.println(n);
  }

}

El codigo no esta mal, ya que si cambio el cerrojo de synchronized por otro que no sea de tipo Integer, funciona perfectamente, lo que deja entrever que no se puede usar un cerrojo que se vaya a modificar posteriormente (hago un n++ con la n declarada como integer).

Algun tipo de explicacion de por que no puedo usar este cerrojo? Me he llevado una inmensa sorpresa y dolores de cabeza por culpa de este error!!

Un saludo y gracias!

[xustyx]

Código (java) [Seleccionar] Expandir


public class Hilo extends Thread {
    private int tipoHilo;
    private  static Integer n = new Integer(0);
    private int nVueltas;


MMM puede ser porque la variable n es privada de la clase hilo????

No se mucho de java pero supongo que esa variable tendría que ser publica y en el main el print seria asi System.out.println(Hilo.n);

[xoker]

Código (java) [Seleccionar] Expandir


public class Hilo extends Thread {
    private int tipoHilo;
    private  static Integer n = new Integer(0);
    private int nVueltas;


MMM puede ser porque la variable n es privada de la clase hilo????

No se mucho de java pero supongo que esa variable tendría que ser publica y en el main el print seria asi System.out.println(Hilo.n);

Para nada... la encapsulacion de datos es hacerlas privadas aun asi no interfiere en el resultado que sea publica.

[~ Yoya ~]

Fixed

Código (java,13) [Seleccionar] Expandir


public class Hilo extends Thread {
private int tipoHilo;
private static Integer n = new Integer(0);
private int nVueltas;

public Hilo(int nVueltas, int tipoHilo) {
this.nVueltas = nVueltas;
this.tipoHilo = tipoHilo;
}

public void run() {

synchronized (n) {

switch (tipoHilo) {
case 0:
for (int i = 0; i < nVueltas; i++)
n = n + 1;

;
break;
case 1:
for (int i = 0; i < nVueltas; i++)
n = n - 1;
;
break;
}
}
}

public static void main(String[] args) throws Exception {
Hilo p = new Hilo(10000, 0);
Hilo q = new Hilo(10000, 1);
p.start();
q.start();
p.join();
q.join();
System.out.println(n);
}

}


La razón porque al principio no funcionaba es porque habías puesto el keyword synchronized dentro del un loop. Cuando se iniciaba un ciclo en el loop, uno de los 2 thread tomaba el lock y el otro esperaba a que liberen el lock.

El problema esta en que el lock se libera cuando termina cada ciclo (son 10,000 ciclos como haz indicado), y puede pasar 2 cosas:

• El thread que habia tomado el lock toma el lock de nuevo
• El thread que estaba esperando que liberen el lock toma el lock.

Por lo tanto, en un momento la variable se puede incrementar y en el siguiente puede que disminuya, y en el siguiente puede incrementarse o disminuirse, y asi.

Ahora al poner el keyword al inicio de todo el metodo, el lock se libera solo cuando el metodo se termine de ejecutar, y cuando se termina de ejecutar, entonce el segundo thread puede tomar el lock ya que se ha liberado.

Saludos.

[xoker]

Fixed

Código (java,13) [Seleccionar] Expandir


public class Hilo extends Thread {
private int tipoHilo;
private static Integer n = new Integer(0);
private int nVueltas;

public Hilo(int nVueltas, int tipoHilo) {
this.nVueltas = nVueltas;
this.tipoHilo = tipoHilo;
}

public void run() {

synchronized (n) {

switch (tipoHilo) {
case 0:
for (int i = 0; i < nVueltas; i++)
n = n + 1;

;
break;
case 1:
for (int i = 0; i < nVueltas; i++)
n = n - 1;
;
break;
}
}
}

public static void main(String[] args) throws Exception {
Hilo p = new Hilo(10000, 0);
Hilo q = new Hilo(10000, 1);
p.start();
q.start();
p.join();
q.join();
System.out.println(n);
}

}


La razón porque al principio no funcionaba es porque habías puesto el keyword synchronized dentro del un loop. Cuando se iniciaba un ciclo en el loop, uno de los 2 thread tomaba el lock y el otro esperaba a que liberen el lock.

El problema esta en que el lock se libera cuando termina cada ciclo (son 10,000 ciclos como haz indicado), y puede pasar 2 cosas:

• El thread que habia tomado el lock toma el lock de nuevo
• El thread que estaba esperando que liberen el lock toma el lock.

Por lo tanto, en un momento la variable se puede incrementar y en el siguiente puede que disminuya, y en el siguiente puede incrementarse o disminuirse, y asi.

Ahora al poner el keyword al inicio de todo el metodo, el lock se libera solo cuando el metodo se termine de ejecutar, y cuando se termina de ejecutar, entonce el segundo thread puede tomar el lock ya que se ha liberado.

Saludos.

Que? no es como dices, porque tal como tu comentas se ejecutaria todo de manera secuencial, mientras que lo que interesa es el paralelismo lo maximo posible para incrementar el rendimiento.

Poner el synchronized dentro del bucle no tiene problema, ya que la sección critica es el contador (la variable n).

El problema del codigo que puse, es que si utilizas un objeto como cerrojo y lo usas, este se modifica y sus propiedades como cerrojo se pierden y se produce el dichoso entrelazado.

Como dije en el primer comentario, si en vez de poner el cerrojo el objeto n de la clase integer ponemos cualquier otro objeto y este no se modifica en ningun momento, el codigo funcionara perfectamente y no existira entrelazado.

Por cierto, lo de poner el synchronized en el switch es una burrada enorme  ... porque la gracia de este codigo es ejecutarlo de forma concurrente mientras que tu manera le quita toda la concurrencia posible...

[~ Yoya ~]

Lo importante, es hacer las cosas bien, entender porque todo esta donde esta. Toma el IDE, y debuguea el codigo y observa como se comporta.

Te recomiendo que trates de leer esta parte:

La razón porque al principio no funcionaba es porque habías puesto el keyword synchronized dentro del un loop. Cuando se iniciaba un ciclo en el loop, uno de los 2 thread tomaba el lock y el otro esperaba a que liberen el lock.

El problema esta en que el lock se libera cuando termina cada ciclo (son 10,000 ciclos como haz indicado), y puede pasar 2 cosas:

• El thread que habia tomado el lock toma el lock de nuevo
• El thread que estaba esperando que liberen el lock toma el lock.

Por lo tanto, en un momento la variable se puede incrementar y en el siguiente puede que disminuya, y en el siguiente puede incrementarse o disminuirse, y asi.

A read / write lock is more sophisticated lock than the Lock implementations shown in the text Locks in Java. Imagine you have an application that reads and writes some resource, but writing it is not done as much as reading it is. Two threads reading the same resource does not cause problems for each other, so multiple threads that want to read the resource are granted access at the same time, overlapping. But, if a single thread wants to write to the resource, no other reads nor writes must be in progress at the same time

In computer science, the first and second readers-writers problems are examples of a common computing problem in concurrency. The two problems deal with situations in which many threads must access the same shared memory at one time, some reading and some writing, with the natural constraint that no process may access the share for reading or writing while another process is in the act of writing to it. (In particular, it is allowed for two or more readers to access the share at the same time.)

La seguridad en hilos es un concepto de programación aplicable en el contexto de los programas multihilos. Una pieza de código es segura en cuanto a los hilos si funciona correctamente durante la ejecución simultánea de múltiples hilos. En particular, debe satisfacer la necesidad de que múltiples hilos accedan a los mismos datos compartidos, y la necesidad de que una pieza compartida de datos sea accedida por solo un hilo en un momento dado.

Saludos.

[lnvisible]

Ninguna de las cosas que habéis puesto me funciona 

Esto sí:

Código (java) [Seleccionar] Expandir

public class Main {

private static Integer n = new Integer(0);
private static Operator op;

static class Operator {

public synchronized void operate (int tipoHilo) {
switch (tipoHilo) {
case 0:
n = n + 1;
break;
case 1:
n = n - 1;
break;
}
}
}

public static class Hilo extends Thread {

private int tipoHilo;

private int nVueltas;

public Hilo(int nVueltas, int tipoHilo) {
this.nVueltas = nVueltas;
this.tipoHilo = tipoHilo;
}

public void run () {
for (int i = 0; i < nVueltas; i++)
op.operate(tipoHilo);
}

}

public static void main (String[] args) throws Exception {
op = new Operator();
Hilo p = new Hilo(10000, 0);
Hilo q = new Hilo(10000, 1);
p.start();
q.start();
p.join();
q.join();
System.out.println(n);
}
}


[xoker]

Vaya... ambos estais comentiendo errores importantes, muy importantes.

Estais convierto un codigo que se ejecuta en paralelo en un codigo secuencial. Yoyas me puedes poner todas las definiciones que quieras, yo entiendo a la perfeccion tu codigo, no esta mal, funciona, pero funciona de manera sencuencia mientras que el mio funciona de manera paralela por lo que aprovecha todos los nucleos de la CPU mientras que el tuyo solo uno.

Da la casualidad de que en este caso, mi codigo solo realiza una operacion dentro de los cases del switch, pero contra mas operaciones independientes tenga, mas rapido seria con respecto al tuyo, ya que tu solo haces un case del switch a la vez mientras que yo hago los dos al mismo tiempo. Con ese codigo te suspenden rapido en cualquier asignatura de programacion concurrente.

Vuelvo a repetir, que si mi codigo se cambia el esta parte:

Código [Seleccionar] Expandir

switch(tipoHilo){
       case 0:
        for(int i=0; i<nVueltas; i++)
        synchronized(n){ n=n+1;};
        break;
       case 1:
        for(int i=0; i<nVueltas; i++){
        synchronized(n){n=n-1;};
        break;
     }

por:
//Supongamos que antes se ha declarado esto:
//Object cerrojo = new Object();

Código [Seleccionar] Expandir

switch(tipoHilo){
       case 0:
        for(int i=0; i<nVueltas; i++)
        synchronized(cerrojo){ n=n+1;}; // <--- Cambio de cerrojo
        break;
       case 1:
        for(int i=0; i<nVueltas; i++){
        synchronized(cerrojo){n=n-1;}; // <--- Cambio de cerrojo
        break;
     }

El codigo funciona perfectamente y en paralelo, lo unico que no comprendo es el porque cuando utilizo un objeto como cerrojo (el integer n en el primer ejemplo) y le cambio su valor, este pierde su cualidad de cerrojo... nada mas!!

[~ Yoya ~]

Me hubiera alegrado que en vez de decir que entiendes mi código, hubieras dicho que entiendes y comprendes la definiciones que deje...


Brother, tu código no funciona con "cerrojo" del tipo int o de cualquier tipo. Tu codigo debe imprimir 0, ya que primero se incrementa a 10,000 y luego descrementa a 0, en cambio retorna 10,000.

Otra cosa, las definiciones no es para que entiendas mi código, el código que puse es lo de menos pero te haz centrado hay. Las definiciones que deje es para que entiendas que si varios threads están modificando el mismo recurso a la vez, te causara conflictos.

switch(tipoHilo){
        case 0:
           for(int i=0; i<nVueltas; i++)
              synchronized(cerrojo){ n=n+1;}; // <--- Cambio de cerrojo
           break;
        case 1:
           for(int i=0; i<nVueltas; i++){
              synchronized(cerrojo){n=n-1;}; // <--- Cambio de cerrojo
           break;
      }

Por la llave del case 1 después del for, hace que se ejecute el for 1 vez, ya que va a procesar el break porque este esta dentro de la llaves.


Aqui esta el famoso codigo con cerrojo del tipo objeto,mira que no funciona:

Código (java,14,17,18,21,22) [Seleccionar] Expandir

package main;


public class Hilo extends Thread {
    private int tipoHilo;
    private  static Integer n = new Integer(0);
    private int nVueltas;

    public Hilo(int nVueltas, int tipoHilo)
    {this.nVueltas=nVueltas; this.tipoHilo=tipoHilo;}

    public void run()
    {
      Object cerrojo = new Object();
      switch(tipoHilo){
        case 0:
        for(int i=0; i<nVueltas; i++)
        synchronized(cerrojo){ n=n+1;};
        break;
        case 1:
        for(int i=0; i<nVueltas; i++)
        synchronized(cerrojo){n=n-1;};
        break;
      }
    }

  public static void main(String[] args)
      throws Exception
  {
      Hilo p = new Hilo(10000, 0);
      Hilo q = new Hilo(10000, 1);
      p.start();
      q.start();
      p.join();
      q.join();
      System.out.println(n);
  }

}

[xoker]

Me hubiera alegrado que en vez de decir que entiendes mi código, hubieras dicho que entiendes y comprendes la definiciones que deje...


Brother, tu código no funciona con "cerrojo" del tipo int o de cualquier tipo. Tu codigo debe imprimir 0, ya que primero se incrementa a 10,000 y luego descrementa a 0, en cambio retorna 10,000.

Otra cosa, las definiciones no es para que entiendas mi código, el código que puse es lo de menos pero te haz centrado hay. Las definiciones que deje es para que entiendas que si varios threads están modificando el mismo recurso a la vez, te causara conflictos.

Por la llave del case 1 después del for, hace que se ejecute el for 1 vez, ya que va a procesar el break porque este esta dentro de la llaves.

Pero vamos a ver hombre de dios... Cuando dos threads acceden al mismo recurso compartido y lo modifican, se produce una cosa que se llama indeterminacion, que significa que el resultado que va a dar va a ser diferente cada vez. Para controlar esta indeterminacion la teoria es acceder al recurso compartido de forma atomica, es decir, que solo acceda de uno en uno a la variable compartida, para hacer esto se emplean varias tecnicas diferentes, entre otras (y la peor en rendimiento) es el synchronized. En el codigo anterior que yo puse, la llave esa se me colo al copiar, fallo mio, .

Aqui esta el famoso codigo con cerrojo del tipo objeto,mira que no funciona:

Código (java,14,17,18,21,22) [Seleccionar] Expandir

package main;


public class Hilo extends Thread {
   private int tipoHilo;
   private  static Integer n = new Integer(0);
   private int nVueltas;

   public Hilo(int nVueltas, int tipoHilo)
   {this.nVueltas=nVueltas; this.tipoHilo=tipoHilo;}

   public void run()
   {
     Object cerrojo = new Object();
     switch(tipoHilo){
       case 0:
        for(int i=0; i<nVueltas; i++)
        synchronized(cerrojo){ n=n+1;};
        break;
       case 1:
        for(int i=0; i<nVueltas; i++)
        synchronized(cerrojo){n=n-1;};
        break;
     }
   }

 public static void main(String[] args)
     throws Exception
 {
     Hilo p = new Hilo(10000, 0);
     Hilo q = new Hilo(10000, 1);
     p.start();
     q.start();
     p.join();
     q.join();
     System.out.println(n);
 }

}

Este codigo refleja que no sabes controlar la exclusion mutua, no hay problema, yo te enseño como se hace, pero no me digas que lo mas basico de programacion concurrente no se puede hacer, un "lo desconozco" es mejor que una mentira.

Para que te quede claro, el primer error y mas bestial que he visto en tu codigo es que declaras el objeto dentro del void run() ¿?¿?¿?¿?!!! Eso lo que hace es que cada hilo tenga su propio cerrojo, por tanto no se podra controlar la exclusion mutua, hay que declararlo como un atributo de la clase siendo de tipo static, para que todos los objetos compartan el mismo valor.

Aqui te pongo el codigo original, copiado y pegado cambiando solo el cerrojo y la llave y comprobado que funciona, siempre te dara 0 a pesar de todo lo que has comentado del synchronized:

Código [Seleccionar] Expandir

import java.util.concurrent.*;

public class Hilo extends Thread {
   private int tipoHilo;
   private  static Integer n = new Integer(0);
   private int nVueltas;
   private static Object cerrojo = new Object();

   public Hilo(int nVueltas, int tipoHilo)
   {
    this.nVueltas=nVueltas;
    this.tipoHilo=tipoHilo;
   }

   public void run()
   {
     
     switch(tipoHilo){
       case 0:
        for(int i=0; i<nVueltas; i++)
        synchronized(cerrojo){ n=n+1;};
        break;
       case 1:
        for(int i=0; i<nVueltas; i++)
        synchronized(cerrojo){n=n-1;};
        break;
     }
   }

 public static void main(String[] args)throws Exception
 {
     Hilo p = new Hilo(10000, 0);
     Hilo q = new Hilo(10000, 1);
     p.start();
     q.start();
     p.join();
     q.join();
     System.out.println(n);
 }
}

Un saludo y me quede con la duda de el dichoso cerrojo Integer...