Logservice simplificada (de la concurrencia en la práctica) con la función de apagado -- java campo con multithreading campo con thread-safety campo con concurrency campo con logging camp codereview Relacionados El problema

Simplified LogService (from concurrency in practice) with shutdown feature


1
vote

problema

Español

Brian Goetz proporcionado después del código de $(function(){ var pos = $("header").offset().top; $(document).scroll(function(){ if($(this).scrollTop() - 10 > pos) { $('header').addClass("filled"); } else { $('header').removeClass("filled"); } }); }); 3 con la función de apagado:

  $(function(){     var pos = $("header").offset().top;      $(document).scroll(function(){         if($(this).scrollTop() - 10 > pos)         {               $('header').addClass("filled");         } else {            $('header').removeClass("filled");                    }     }); }); 4  

Creo que tengo una variante más sencilla (no entiendo por qué necesitamos contador de reservaciones), revise su seguridad:

  $(function(){     var pos = $("header").offset().top;      $(document).scroll(function(){         if($(this).scrollTop() - 10 > pos)         {               $('header').addClass("filled");         } else {            $('header').removeClass("filled");                    }     }); }); 5  
Original en ingles

Brian Goetz provided following code of LogService with shutdown feature:

public class LogService {     private final BlockingQueue<String> queue;     private final LoggerThread loggerThread;     private final PrintWriter writer;     @GuardedBy("this") private boolean isShutdown;     @GuardedBy("this") private int reservations;      public LogService(Writer writer) {         this.queue = new LinkedBlockingQueue<String>();         this.loggerThread = new LoggerThread();         this.writer = new PrintWriter(writer);     }      public void start() {         loggerThread.start();     }      public void stop() {         synchronized (this) {             isShutdown = true;         }         loggerThread.interrupt();     }      public void log(String msg) throws InterruptedException {         synchronized (this) {             if (isShutdown)                 throw new IllegalStateException(/*...*/);             ++reservations;         }         queue.put(msg);     }      private class LoggerThread extends Thread {         public void run() {             try {                 while (true) {                     try {                         synchronized (LogService.this) {                             if (isShutdown && reservations == 0)                                 break;                         }                         String msg = queue.take();                         synchronized (LogService.this) {                             --reservations;                         }                         writer.println(msg);                     } catch (InterruptedException e) { /* retry */                     }                 }             } finally {                 writer.close();             }         }     } } 

I think that I have simpler variant(I don't understand why we need reservations counter), please review its safety:

public class LogService {     private final BlockingQueue<String> queue;     private final LoggerThread loggerThread;     private final PrintWriter writer;     @GuardedBy("this")     private boolean isShutdown;       public LogService(Writer writer) {         this.queue = new LinkedBlockingQueue<String>();         this.loggerThread = new LoggerThread();         this.writer = new PrintWriter(writer);     }      public void start() {         loggerThread.start();     }      public void stop() {         synchronized (this) {             isShutdown = true;         }         loggerThread.interrupt();     }      public void log(String msg) throws InterruptedException {         synchronized (this) {             if (isShutdown)                 throw new IllegalStateException(/*...*/);             queue.put(msg); // inside synchronized now         }      }      private class LoggerThread extends Thread {         public void run() {             try {                 while (true) {                     try {                         synchronized (LogService.this) {                             if (isShutdown && queue.isEmpty())                                 break;                         }                         String msg = queue.take();                         writer.println(msg);                     } catch (InterruptedException e) { /* retry */                     }                 }             } finally {                 writer.close();             }         }     } } 
              

Lista de respuestas

1
 
vote

Asunto de la concurrencia

Creo que una parte crítica se encuentra aquí:

  GRID_MAX_X9  

Usted consulta isshutdown y la cola.isempty (), pero ya no puede garantizar este estado si sale del monitor y accede a la cola después.

Después de eso, también no puede asegurarse de la orden de salida (Imprimir) que esté especificada por la cola.

ambos deben hacerse en el monitor.

Guardedby Obsoleto

Además, no creo que la anotación Guardedby sea necesaria como ISSHUTDN, ya solo se solicita y modifica dentro del monitor (sincronizado (esto)).

  GRID_MAX_Y0  

Escritor

No sé si está destinado a cerrar la impresora después de que la cola esté vacía. Esto causará un problema al reiniciar el logthread. Los impresores cerrados pueden no funcionar más, lo que depende del escritor subyacente que interpreta la operación "Close ()":

  GRID_MAX_Y1  

Pruebe con los recursos

Si desea cerrar los recursos correctamente, debe usar el siguiente constructo:

  GRID_MAX_Y2  

Otro punto es que debe tener una simetría para crear y cerrar un recurso. Su registro de registros ahora es responsable de cerrar el escritor, pero no lo crea. O bien proporciona una fábrica para el recurso, por lo que el registro de registro es responsable de crear y cerrarlo o el usuario del registro de registros que pasa en el escritor debe ser consciente de la creación y el cierre. Sin embargo, la mayor proximidad para esas operaciones es el objetivo.

Manejo del estado

Actualmente usted Micro Gestione el manejo del estado (funcionando / detenido). Debe considerar implementar un verdadero patrón estatal como un gran concepto para aclarar cuando se permite el método ser llamado.

Tienes algunas cosas semánticas que encuentro confuso. El registro se permite solo cuando el servicio no se detiene, pero siempre puede comenzar y detener el servicio.

interrupción de hilo

Su hilo no debe detenerse a través del método de interrupción. Intente reformular su comunicación con el hilo para detenerse sin usar un mecanismo tan duro que arroja una excepción.

Reiniciando

Permite iniciar el loggerthread, aunque esto causará una excepción. Esperaría noop o una excepción específica de dominio, ya que la persona que llama no está interesada en una excepción específica de hilo. ¿Debería la persona que llama saber que un Thed está trabajando? Otra posibilidad es comenzar con un nuevo hilo. Ya maneja el estado, por lo que la funcionalidad obvia para llamar al inicio (): el método en un registro de registro detenido es para iniciarlo nuevamente.

Opinión personal

Por lo que esto es que no confiaría ni a una ni la otra implementación, ya que están plagadas con ambigüedades semánticas, problemas de concurrencia y micro gestión del estado. Sugiero replicar el concepto con el manejo del estado real y una definición de monitor adecuada.

Código

Estado

El patrón de estado cubrirá tres métodos:

  GRID_MAX_Y3  

Logservice

El registro de registros delegará todas las llamadas públicas al patrón estatal. Proporciona una cola que atravesará los mensajes de registro.

Inicialmente, el registro de registros está en el estado "statestopped".

Ves que cambiamos de una cola de bloqueo a una cola normal que no es segura de hilo. Pero eso está bien, ya que el registro de registro sirve como un monitor a su lado, al lado de las otras dos variables de la leña y el estado.

LogTarget puede acceder al concurrente, pero esta no es nuestra responsabilidad y solo podemos garantizar la implementación correcta por convención.

  GRID_MAX_Y4  

statestopped

Como el patrón de estado tiene un objeto de contexto (aquí LogService) Este objeto de contexto se utilizará como monitor (sincronizado (logservice.se)).

Mi implementación acepta los mensajes de registro Ponlo, no notificará a ningún consumidor.

La parada de llamadas () será un NOOP. Si su sabor es lanzar una excepción, hágalo.

Inicio de llamadas () Instanciará un nuevo hilo y lo iniciará. Después de eso, el estado interno se cambia para ser "estancamiento".

  GRID_MAX_Y5  

Statuunning

Como el patrón de estado tiene un objeto de contexto (aquí LogService) Este objeto de contexto se utilizará como monitor (sincronizado (logservice.se)).

Inicio de llamada () conducirá a un NOOP. Aquí tienes la opción de lanzar una excepción también.

El registro de llamadas () aceptará nuevos mensajes de registro, pero como el servicio sabe que está disponible un hilo (StatuLunning) que puede esperar a que se notifique nuevos mensajes.

Parada de llamadas () apagará de manera segura el hilo por c MRICULAR UNA SOLICITUD DE PARADA.

Cambiaremos el estado interno del registro registrador dentro del monitor y esperando hasta que el hilo termina fuera del monitor.

  GRID_MAX_Y6  

LogProcessAsreadRead

Este hilo, mientras que la refactorización parece ser cada vez más un artefacto que solicitará los mensajes de registro desde algún lugar y pasa este mensaje de registro a algún lugar.

Aquí nuevamente: el hilo en sí mismo es el monitor sobre "hueso en punto" y la fuente de operaciones críticas.getNextQueCeDessage () y Target.log (mensaje) que no debe ser interceptado.

  GRID_MAX_Y7  

LogSource, LogTarget

Estos no son necesarios, pero muestran la abstracción inherente, el hilo hace uso. Por lo tanto, un registro de registro es, por supuesto, un logsource, pero también puede ser un logTarget.

  GRID_MAX_Y8  

 

Concurrency issue

I think one critical part is found here:

synchronized (LogService.this) {     if (isShutdown && queue.isEmpty()) break; } --> ... <-- String msg = queue.take(); --> ... <-- writer.println(msg); 

You query isShutdown and queue.isEmpty() but you cannot ensure this state anymore if you leave the monitor and access the queue afterwards.

After that you also cannot ensure the output order (println) that is specified by the queue.

Both should be done in the monitor.

GuardedBy obsolete

Furthermore I do not think that the GuardedBy annoptation is neccessary as isShutdown is already only requested and modified within the monitor (synchronized(this)).

@GuardedBy("this") private boolean isShutdown; 

Writer

I do not know if it is intended to close the PrinterWriter after the queue is empty. This will cause a problem when restarting the LogThread. Closed PrintWriters may not work anymore which depends on the underlying Writer that interpretes the "close()"-operation:

PrintWriter pw = new PrintWriter(new FileOutputStream("hello.txt")); pw.close(); pw.println("Hallo"); // will produce no output in file 

try with resources

If you want to close resources properly you should use following construct:

try (PrintWriter writer = new PrintWriter(...)) {     ... } catch (...) {     ... } 

Another point is that you should have a symmetry in creating and closing a resource. Your LogService is now responsible to close the Writer but it doesn't create it. Either you provide a factory for the resource so the LogService is responsible for creating and closing it OR the user of the LogService that passes in the Writer should be aware of creation and closing. Nevertheless the maximum proximity for those operations is the goal.

State handling

Currently you micro manage the state handling (running/stopped). You should consider implementing a real state pattern as a great concept to make clear when which method is allowed to be called.

You have some semantic things I find confusing. Logging is allowed only when the service is not stopped but you may always start and stop the service.

Thread interrupt

Your thread should not be stopped through the interrupt method. Try to reformulate your communication with the thread to stop without using such harsh mechanism that throws an exception.

Restarting

You do allow starting the loggerThread although this will cause an exception. I would expect either NOOP or a domain specific exception as the caller is not interested in a thread specific exception. Should the caller even know that a thead is working? Another possibility is to start over with a new thread. You already handle state so the obvious functionality for calling the start()-method on a stopped LogService is to start it again.

Personal opinion

As this is I would trust neither one nor the other implementation as they are riddled with semantic ambiguities, concurrency issues and micro management of state. I suggest to rethink the concept with real state handling and a proper monitor definition.

Code

State

The state pattern will cover three methods:

private interface State {     void start();     void log(String message);     void stop(); } 

LogService

The LogService will delegate all public calls to the state pattern. It provides a queue that will buffer log messages.

Initially the LogService is in the state "StateStopped".

You see that we changed from a blocking queue to a normal queue that is not thread-safe. But that is ok as the LogService serves as a monitor around it beside the other two variables loggerThread and state.

LogTarget may be access concurrently but this is not our responsibility AND we can only ensure the correct implementation by convention.

public class LogService implements LogTarget, LogSource {      // states     ...      private State state;      private final Queue<String> messageQueue;     private LogProcessorThread loggerThread;     private final LogTarget logTarget;       public LogService(LogTarget logTarget) {         this.messageQueue = new ArrayDeque<String>();         this.logTarget = logTarget;         this.state = new StateStopped();     }      public void start() {         this.state.start();     }      public void stop() {         this.state.stop();     }      public void log(String message) {         this.state.log(message);     }      public String getNextQueuedMessage() {         synchronized (this) {             return this.messageQueue.poll();         }     }  } 

StateStopped

As the state pattern has a context object (here LogService) this context object will be used as the monitor (synchronized(LogService.this)).

My implementation accepts log messages put it will not notify any consumer.

Calling stop() will be a NOOP. If your flavour is to throw an exception do that.

Calling start() will instantiate a new thread and start it. After that the internal state is changed to be "StateRunning".

private class StateStopped implements State {      @Override     public void start() {         synchronized (LogService.this) {             loggerThread = new LogProcessorThread(LogService.this, logTarget);             loggerThread.start();             state = new StateRunning();         }     }      @Override     public void log(String message) {         synchronized (this) {             messageQueue.add(message);         }     }      @Override     public void stop() {         // ignore or throw IllegalStateException... your choice     }  } 

StateRunning

As the state pattern has a context object (here LogService) this context object will be used as the monitor (synchronized(LogService.this)).

Calling start() will lead to a NOOP. Here you have the choice to throw an exception as well.

Calling log() will accept new log messages but as the service knows that a thread is available (StateRunning) that may wait for new messages it is notified.

Calling stop() will safely shutdown the thread by communicating a stop request.

We will change the internal state of the LogService within the monitor and waiting until the thread terminates outside the monitor.

private class StateRunning implements State {      @Override     public void start() {         // ignore or throw IllegalStateException... your choice     }      @Override     public void log(String message) {         synchronized (this) {             messageQueue.add(message);             loggerThread.awakeOnWait(); // this is more a callback or an observer pattern         }     }      @Override     public void stop() {         Thread lastBreath = null;         synchronized (LogService.this) {             lastBreath = loggerThread;             loggerThread.requestStop();             loggerThread = null;             state = new StateStopped();         }         try {             lastBreath.join();         } catch (InterruptedException e) {         }     }  } 

LogProcessorThread

This thread while refactoring seems to become more and more an artefact that will ask for log messages from somewhere and passes these log message to somewhere.

Here again: The thread itself is the monitor over "shouldStop" and the critical operations source.getNextQueuedMessage() and target.log(message) that should not be intercepted.

public class LogProcessorThread extends Thread {      private LogTarget target;     private LogSource source;     private boolean shouldStop = false;      public LogProcessorThread(LogSource source, LogTarget target) {         this.target = target;         this.source = source;     }      public void run() {         boolean expectedMoreMessages = true;         while (expectedMoreMessages) {             expectedMoreMessages = logNextMessage();         }     }      public synchronized void requestStop() {         this.shouldStop = true;         this.notify();     }      public synchronized void awakeOnWait() {         this.notify();     }      public synchronized boolean logNextMessage() {          boolean shouldStop = this.shouldStop; // keep the state stable in this method invocation          String message = source.getNextQueuedMessage();         if (message != null) {             target.log(message);         }         if (message == null && !shouldStop) {             try {                 this.wait();             } catch (InterruptedException e) {                 throw new RuntimeException("unexpected", e);             }         }         boolean expectMoreMessages = message != null || !shouldStop;         return expectMoreMessages;     }  } 

LogSource, LogTarget

These are not neccessary but they show the inherent abstraction the thread makes use of. So A LogService is of course a LogSource but it can also be a LogTarget.

public interface LogSource {     String getNextQueuedMessage(); }  public interface LogTarget {     public void log(String message); } 
 
 
0
 
vote

Estoy de acuerdo con @OOPEXPERT que ni la implementación se ve particularmente agradable.

Puedo ver algunas formas de mejorar esto:

  • isshutdown probablemente debería hacerse volátil; O si realmente desea garantizar un comportamiento consistente, tal vez considere usar AtomicBoolean en su lugar.
  • LinkedBlockingQueue es un hilo seguro, por lo que mover la oferta dentro del bloque sincronizado parece redundante y aumenta la cantidad de argumento de bloqueo en esto.
  • Si usa un AtomicBoolean, no necesitarías un bloque sincronizado En absoluto para hacer cola de nuevos mensajes.
  • Alternativamente, podría considerar usar un bloqueo de readwrite y usar el WritElock solo al imprimir mensajes. Esto tiene la ventaja de permitir que múltiples subprocesos agregue mensajes al mismo tiempo con un bloqueo de lectura.
  • Los hilos del escritor podrían drenar la cola e imprimir todo en lugar de simplemente tomar un artículo. Esto podría terminar siendo más eficiente.
  • El bloqueo solo debe adquirirse si la cola no está vacía. Ahora, básicamente, todos los hilos del escritor son esencialmente adquiriendo un bloqueo en cada iteración, cuando posiblemente, en su mayoría, en su mayoría no lo necesitarían a menos que la cola no esté vacía.
 

I agree with @oopexpert that neither implementation looks particularly nice.

I can see a few ways to improve this:

  • isShutdown should probably be made volatile; or if you really want to ensure consistent behavior, maybe consider using AtomicBoolean instead.
  • LinkedBlockingQueue is thread safe, so moving the offer inside the synchronized block seems redundant and increases the amount of lock contention on this.
  • If you use an AtomicBoolean, you wouldn't need a synchronized block at all for queueing new messages.
  • Alternatively, you could consider using a ReadWrite lock and use the writelock only when printing messages. This has the advantage of allowing multiple threads to add messages at the same time with a read lock.
  • The writer threads could drain the queue and print everything instead of merely taking one item. This could end up being more efficient.
  • The lock should only be acquired if the queue is not empty. Now basically all the writer threads are essentially acquiring a lock on every iteration, when arguably they mostly would not need it unless the queue is not empty.
 
 

Relacionados problema

4  Posibles peligros de desbordamiento de búfer en el programa de análisis de registro C  ( Possible buffer overflow dangers in c log parsing program ) 
Después de horas de trabajo, finalmente terminé mi primer programa de análisis de registro C! (Anteriormente fue un guión bash, ahora es C). ¡Aunque creo qu...

3  Archivo de registro diferido Cerrar  ( Deferred log file close ) 
Mi código funciona en que se compila, y cuando se ejecuta, escribe un archivo de registro utilizando un escritor búfer. pero me pregunto si: Soy correcto...

5  Iniciar sesión en un archivo vs udp  ( Logging to a file vs udp ) 
Estoy tratando de entender dónde mi implementación de rubíes de esto falla y con la esperanza de que un experto arroje algo de luz sobre esta situación. He in...

4  Iniciar sesión en la base de datos con el núcleo ASP.NET  ( Logging into database with asp net core ) 
En el Container9 de mi aplicación ASP.NET Core Quiero configurar el 99887766655443330 para que inicie sesión en la base de datos. Por lo tanto, creo despu...

6  Uso de la excepción al registro de la pila de ejecución  ( Use of exception to log execution stack trace ) 
He estado de ida y vuelta con un colega sobre el uso de Throwable.fillInStackTrace . Este Log clase está destinado a envolver el Fachada de registro simpl...

6  Autorización PBKDF2  ( Pbkdf2 authorization ) 
He descubierto que el uso de contraseñas hashed con sales es una idea mucho mejor que MD5 / SHA256, por lo que no los estoy pulsando con PBKDF2. Sin embargo, ...

1  Encapsulando el registro y lanzamiento de excepciones a un método. ¿Mala práctica?  ( Encapsulating logging and throwing exceptions into a method bad practice ) 
Ejemplo de código original if (var1 == null) { String msg = "var 1 is null"; logger.log(msg); throw new CustomException(msg); } if (var2 == nul...

5  Método / Parámetro Tracer  ( Method parameter tracer ) 
Estoy escribiendo una clase que puede registrar la actividad de una solicitud durante el tiempo de ejecución. El plan es que la cadena 9988776665544330 se e...

2  Evitando la duplicación de código en múltiples, excepto bloques en la clase de registrador [cerrado]  ( Avoiding code duplication in multiple except blocks in logger class ) 
cerrado. Esta pregunta es off-topic . Actualmente no está aceptando respuestas. ¿Quieres ...

5  ¿Cómo mejoro este mecanismo de tala?  ( How do i improve this logging mechanism ) 
Hay bastantes cosas que estoy considerando: Tendré que comprobar si null valores Tendré serios problemas que lo persisten en la base de datos. ¿Cómo...




© 2022 respuesta.top Reservados todos los derechos. Centro de preguntas y respuestas reservados todos los derechos