Etiquetas
Ayer a última hora me encontré con este código:
Quité algunas líneas para que no se pudiese deducir la empresa, que no es importante. Tampoco el programador que lo hizo, no es mi intención criticar a nadie en particular sino a una mala práctica de solucionar problemas de concurrencia con «ideas geniales» que no lo son tanta.
El código anterior recibe consultas B2B, graba los resultados en un fichero (por requerimientos de negocio) y luego lee desde esos mismos ficheros para enviar la respuesta a la agencia que hizo la consulta.
Es un software que factura millones de euros al año y pasó semanas de tests y QA. No se descubrió el problema hasta que una de los clientes optimizó sus consultas enviando varias en paralelo. Allí descubrieron que obtenían resultados erróneos: eran duplicados (con un riesgo financiero bastante importante).
Moraleja 1: Los errores de concurrencia son difíciles de encontrar y cuando aparecen suelen ser difíciles de debuguear, ten mucho cuidado antes de implementar soluciones ad-hoc.
El código intenta resolver el problema de concurrencia de varios procesos que podrían escribir en el mismo fichero verificando primero si el fichero existe, si es así incrementa el contador -que forma parte del nombre- hasta que encuentra uno que no coincida.
Además de detalles de eficiencia tiene un problema grave, las operaciones de verificación (file_exists) y creación del fichero (fopen) no son atómicas. Eso significa que varios procesos concurrentes (con una o varias CPUs) verifican, les da falso a todos y luego todos escriben en el mismo fichero. Una race condition de libro.
Moraleja 2: Si has tardado en encontrar el problema o no entiendes la importancia de operaciones atómicas ya deberías estar aprendiendo más de concurrencia.
Afortunadamente entre los tres que estábamos analizando el código en plan «emergencia» y por vídeoconferencia (la captura es del chat de Skype de Linux) desde países diferentes nos dimos cuenta enseguida del bug y de una solución simple: agregar el PID del proceso al nombre del fichero. Como el PID es único en el sistema y cada proceso individual es secuencial no hay riesgos de que sobreescriban en el mismo fichero.
No es la solución más eficiente (ya lo corregiremos estos días con más tranquilidad) pero es una solución segura y que podía implementarse en pocos minutos y sin efectos colaterales que podrían afectar a la lógica del programa.
Además, el bug podría haber sido menos perjudicial si en vez de escribir los resultados en el fichero y luego leerlos para responder se hubiese enviado directamente la respuesta ya almacenada en el proceso local.
Moraleja 3: Si no te has dado cuenta de la diferencia entre datos locales y compartidos y los problemas que pueden generar estos últimos, ponte ya a aprender de concurrencia.
Moraleja 4: Los problemas de concurrencia son muchos más habituales de los que crees y las soluciones no son tan obvias como parecen a primera vista.
Obviamente tengo que recomendar el libro que ya sabéis (y aparece a la izquierda 😉 ) pero ni aún así se puede asegurar que no cometerás errores, se requiere práctica y ser muy cuidadoso.
Ricardo Galli, de software 
Si te contaste lo que yo vi una vez… Un GENIO (lo pongo con mayúsculas por que se lo merece) no sabía usar MySQL y creó una base de datos con todos los campos Varchar y sin índices.
Y claro, la clave primaria no era un autoincrement, entonces el genio creó una cosa llamada «codauto», una función (en un script PHP diseñado para la versión 4) que leía el último ID de la base de datos, le sumaba uno y creaba la SQL del insert con el ID ya relleno. Esta aberración se aplicaba a absolutamente todos los inserts en todas las tablas, sin excepción.
No sólo era una puta barbaridad como un piano de grande, es que además como la base de datos era MySQL mysam (texto plano si no me falla la memoria) leía mucho más rápido de lo que escribía, por lo que el margen de error para que ocurriera una tragedia era de más de 1 segundo.
Lo mejor de toda esta situación digna de aparecer en un libro de malas prácticas era la parte usuario, que jamás notificó el error hasta pasados varios años, se limitaban a darle a «guardar» una segunda vez si fallaba la primera.
Lo más bruto de todo fue cuando me di cuenta e intenté arreglarlo, resulta que era imposible hacer las cosas bien (la función se usaba en 4 aplicaciones distintas y yo sólo podía tocar una, y además la que yo tocaba usaba dos BBDD distintas y yo sólo podía modificar una), así que hice una puta barbaridad aún más grande (pero que funcionó bien… bueno… más o menos bien). No voy a entrar en detalles, sólo diré que la función «mt_rand» cobró un protagonismo que no se merecía y que los usuarios empezaron a quejarse de los números de expediente, que crecían de manera descontrolada.
Al menos dejaron de quejarse de fallos de concurrencia…