Fix DBhandler exception when get_settings fails

[giosava94]

Now, when get_settings raises a ValueError, the get_logger function catches that error and applies a default value.

The DBHandler's constructor initializes the _logger and _engine instance variables to None by default. This way, when the instance creation fails, the DBHandler disposes the engine connection only if _engine is not None (execution of the __del__ method).

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[jacogasp]

Qual è il vantaggio di questa cosa? Il problema non sta in get_settings? Che sia dentro alla funzione get_logger credo importi poco

[giosava94]

La funzione get_settings c'era già nell'init, non l'ho aggiunta. Semplicemente era dopo get_logger. L'ho messa prima di get_logger per 2 motivi:

• se ci sono problemi con settings, crasha prima di chiamare get_logger, invece di dover gestire l'eccezione nel get_logger e poi cmq crashare.
• avendo qui un get_settings funzionante, posso passare direttamente log_level (risparmiando una seconda chiamata a get_settings)

[jacogasp]

Mmm non mi piace però, perché così devi passare il log level ogni volta che chiami get_logger, e a quel punto puoi trovarti con due log level diversi. Io lo terrei dentro punto e basta.

Invece che ci sta che crashi alla 37, però se get_settings() crasha mi sta bene che crashi pure DBHandler.__init__

A questo punto il problema è che viene chiamato il distruttore __del__ anche se è crashato __init__, che non ha molto senso a mio avviso

[giosava94]

il log_level è rimasto opzionale. Se non è passato si prendo quello di settings (come era prima di questa modifica)

[jacogasp]

Perché sono diventati opzionali? Li lascerei non opzionali, al più si deve rompere l'app se engine non è stato in grado di costruirlo

[giosava94]

Il problema non è nella costruzione ma nell'eliminazione del DBHandler (vedi sotto)

[jacogasp]

Vedi sopra, non vedo perché self._engine dovrebbe essere none

[giosava94]

Se non metti self._engine = None e l'app crasha quando carica i settings o il logger, lui in ogni caso esegue il __del__. Quindi, se non metti self._engine = None nell'__init__ il __del__ crasha perchè non trova la variabile self._engine.

[jacogasp]

Ma infatti il problema sta qui. Deve crashare init e non deve essere essere chiamato il distruttore __del__. L'app deve essere già morta prima ancora di arrivare a __del__

[giosava94]

L'init crasha infatti. Ma cmq viene eseguito il del. Se vuoi rimettiti sul main e fai solo la modifica che proponi (sul get_logger). Vedrai che ti da problemi.

[jacogasp]

https://godbolt.org/z/3MshM74za maledetto python, dobbiamo sistemare questa cosa che è terribile

[giosava94]

Questo vuol dire aggiugere un try except in più nel delete (cosa che tu stesso hai detto essere poco efficiente). Tenere l'if con il valore a none fa la stessa cosa e funziona.

[giosava94]

Adotterei la tecnica del try/except nel distruttore. Butterei via tutte le modifiche di questa pr e terrei solo l'esempio di stackoverflow

Fai come meglio credi. Io non concordo (su vari commenti) ma chiudi pure la PR.

[jacogasp]

Il try except è poco efficiente se lo usi al posto di un "if" e ne fai tanti. Dal momento che qui muore, non è un problema, e di solito questa cosa è buona norma fatta nei distruttori proprio perché ci possono essere eccezioni in fase di distruzione

https://baltig.infn.it/cnafsd/storm-tape/-/blob/main/src/profiler.hpp?ref_type=heads#L102

Lo so che con l'if funziona lo stesso, però implicitamente mi hai detto che DBHandler può esistere senza engine, mentre i due devono avere lo stesso identico lifespan (uno non esiste senza l'altro)

[jacogasp]

Se fosse un linguaggio compilato/fortemente tipato, ora DBHandler.get_engine() ritornerebbe un optional<Engine>

[jacogasp]

Adotterei la tecnica del try/except nel distruttore. Butterei via tutte le modifiche di questa pr e terrei solo l'esempio di stackoverflow

Fai come meglio credi. Io non concordo (su vari commenti) ma chiudi pure la PR.

No non volevo buttare via la PR, volevo solo fare revert e mettere il try/execept nel distruttore del db handler :)

In realtà il try/except direi che comunque ci vuole dentro get_settings()

[jacogasp]

Non mi fa impazzire questo try/except fatto così, anche perché non sappiamo cosa succede se get_settings fallisce per errori diversi da ValueError.

Farei qualcosa del tipo

def get_logger(name):
    formatter = "..."
    stream_handler.setFormatter(formatter)
    logger = logging.getLogger(name)
    logger.addHandler(stream_handler)
    try:
      settings = get_settings()
      logger.setLevel(level=log_level)
    except e:
       logger.setLevel(DEFAULT_LEVEL)
       logger.exception(e)
    return logger