RocketLeague-Taller-de-Programacion-I / TP_RocketLeague

Proyect to Rocket League - FIUBA
0 stars 2 forks source link

Falla en el pop de BlockingQueue #16

Closed LucasWaisten closed 1 year ago

LucasWaisten commented 1 year ago

@leogm99 @mateocapon Hola profres.

Hice un copy paste de la cola bloquiante que Ezequiel subio de la clases. Le hice unos test unitarios para corroborar que funcionara el pop y el push, pero tengo un error en este test:

https://github.com/RocketLeague-Taller-de-Programacion-I/TP_RocketLeague/blob/b9004f212e0dea2451dea5acae4341e3abb120b2/tests/blockingQueueTest.cpp#L53-L69

En el test trato de ver si cuando un thread hace un pop en una cola que esta empty, se queda bloqueado en el pop hasta que desde otro hilo hagan un push. Pero me tira un error en el mutex, es un segmentation fault.

No supe bien si era un error mio tal vez de la clase que hice para testear o de la misma cola. Adjunto la clase testerQueue que me sirve para testar estos casos. https://github.com/RocketLeague-Taller-de-Programacion-I/TP_RocketLeague/blob/b9004f212e0dea2451dea5acae4341e3abb120b2/tests/blockingQueueTest.cpp#L8-L24

leogm99 commented 1 year ago

Hola Lucas, como va? Ahí estoy viendo un par de cosas. Primero que nada en el header de la cola bloqueante falta una directiva ifndef para evitar multiples includes del mismo header, aunque eso no es el problema que estas teniendo en este test.

Tenes un par de errores: hay 2 race conditions sobre el booleano waiting. La primera es que tranquilamente el hilo que ejecute las pruebas checkee el booleano antes de que el hilo Tester lo setee en true. En segundo lugar, por mas de que asegures esa condición, no sabes si los hilos están viendo diferentes partes de sus cachés al leer ese booleano, con lo cual ahi tenes una data race, eso se solucionaría utilizando un std::atomic<bool>. En ultimo lugar y no por eso menos importante (de hecho es clave!), nunca joineas el hilo!!! Deberías joinearlo antes del último REQUIRE.

De todas maneras, el scope del tp es testear el protocolo y parte de la lógica de creación de partidas (basicamente el monitor de games). Estos tests entiendo que no serían 100% necesarios (aunque es un lindo detalle testear estas cosas). Recomendaría que enfoquen a features y a los tests obligatorios!

leogm99 commented 1 year ago

Agrego la versión práctica de como resolver el problema: si joineas el hilo antes del último REQUIRE y haces al booleano un std::atomic<bool>, el test debería correr correctamente

LucasWaisten commented 1 year ago

Efectivamente, funciono el test con los cambios que me mencionaste

Justo tuvimos varios errores por incluir varios múltiples veces el mismo header, así que utilizaré ese consejo para aplicar en las demás clases tmbn.

Por otro lado, también me concentraré en los features y test pedidos. Muchas gracias, Leo