SmtpMailer – odesílá mail 2×

admin@easyweb4u.cz
Backer | 143
+
0
-

Zdar kolegové, mám script pro odesílání emailů. Stává se, dost často, že jsou emaily odeslány 2× včetně zápisu do databáze. Kontrolní test ale ukazuje správný počet odeslaných emailů. Už se s tím mořím… Tady ten script:

<?php
    public function startup() {
        parent::startup();

        $tasks = $this->notice->dataEmailExpiredTask();

        if ($tasks != null) {

            foreach ($tasks as $task) { // po jednotlivych ukolech a terapeutech

                if ($this->notice->allowedEmail($task['firmid'], 'expir_admin') === 1) {

                    $mail = new Message;

                    $mail->setFrom('upozorneni@xxxxxx.cz')
                            ->addTo($task['emailfirm'])
                            ->setSubject('Upozornění na úkol po termínu: ' . $task['calname'])
                            ->setBody('Upozornění na úkol po termínu: ' . $task['calname'])
                            ->setHtmlBody('<div style="position: relative; width: 440px; margin: 20px auto 15px auto; padding: 10px; background: #e6e7e8; box-shadow: 5px 5px 7px #949494;">'
                                    . '<p><span style="text-decoration:underline">Upozornění na úkol po termínu</span><br>'
                                        . 'Ze dne: ' . DateTime::from($task['day'])->format('j. n. Y') . '<br>'
                                        . 'Název: ' . $task['calname'] . '<br>'
                                        . 'Pracovník: ' . $task['name'] . ' ' . $task['surname'] . '<br>'
                                    . '<img style="position:absolute; top:0;right:0;width:60px" src="' . $this->getHttpRequest()->getUrl()->getHostUrl() . '/design/logo.png" alt="logo">'
                                    . '</p>'
                                    . '</div>');

                    $mailer = new SmtpMailer($this->mailerArr);

                    $mailer->send($mail);

                    unset($mail);

                    unset($mailer);

                    $this->test[] = ['admin_email ', $task['calname'], $task['firmid'], 'upozorneni@xxxxxxx.cz', $task['emailfirm']];

                    $this->database->query('INSERT INTO sentemail', ['firmid' => $task['firmid'], 'calendarid' => $task['id'], 'email' => $task['emailfirm']]);

                    unset ($task);

                }

            }

        }

    }
?>
admin@easyweb4u.cz
Backer | 143
+
0
-

Tím chci říct, že pole tasks má správný počet položek a počet cyklů taky souhlasí. Tím jsem si jist. Vypadá to, jako by se ten script spustil opakovaně…

Kamil Valenta
Člen | 765
+
+1
-

Takže máš podezření na paralelní souběh. Nejspíš to tak bude. Jak to tedy máš proti souběhu ošetřeno?
Jak vypadá dataEmailExpiredTask()? Zohledňuje zápisy v sentemail?

Obecně není dobré „něco vytáhnout, dlouho zpracovávat a pak zapisovat někam doběhnutí“. Pokud na to najíždí třeba cron každou minutu, dataEmailExpiredTask() vratí např. ID 1,2,3,4. Poštovní server ale může odesílat celkem pomalu a než to celé doběhne, cron znovu tikne a dataEmailExpiredTask() vratí ID 3,4,5,6.
Tasky #3 a #4 doběhnou z prvního cronu, ale zároveň poběží i z druhého.

admin@easyweb4u.cz
Backer | 143
+
0
-

Kamil Valenta napsal(a):

Takže máš podezření na paralelní souběh. Nejspíš to tak bude. Jak to tedy máš proti souběhu ošetřeno?
Jak vypadá dataEmailExpiredTask()? Zohledňuje zápisy v sentemail?

Obecně není dobré „něco vytáhnout, dlouho zpracovávat a pak zapisovat někam doběhnutí“. Pokud na to najíždí třeba cron každou minutu, dataEmailExpiredTask() vratí např. ID 1,2,3,4. Poštovní server ale může odesílat celkem pomalu a než to celé doběhne, cron znovu tikne a dataEmailExpiredTask() vratí ID 3,4,5,6.
Tasky #3 a #4 doběhnou z prvního cronu, ale zároveň poběží i z druhého.

Předem díky za odpověď. Vůbec jsem netušil nic o nějakém souběhu, ale chápu, že se něco takového může dít. Tady ta metoda:

<?php
public function dataEmailExpiredTask() { // na produkcnim nastavit CRON

        $select = $this->database->table('firm')->fetchAll();

        if ($select != null) {

            $tasks = []; // ukoly pro firmid, terapeut muze mit vic zaznamu (emaily jednotlive pro kazdy ukol)

            $deadline = DateTime::from(date('Y-m-d'))->modifyClone('-1 day');

            foreach ($select as $se) {

                $sel = $this->database->query("SELECT calendar.*, terapeut.name, terapeut.surname, terapeut.email, terapeut.phone FROM calendar "
                        . "LEFT JOIN terapeut ON terapeut.id = calendar.terapeutid "
                        . "LEFT JOIN users ON users.id = terapeut.userid "
                        . "WHERE users.firmid = $se->id "
                        . "AND ((day = '$deadline' AND dayend IS NULL) OR (dayend = '$deadline' AND start IS NULL)) "
                        . "AND calendar.inserter = 1 "
                        . "AND calendar.status = 1 ORDER BY day");

                if ($sel != null) {

                    foreach ($sel as $s) {

                        $row = $this->database->table('firm')->get($se->id);

                        if ($row) {

                            $emailfirm = $row->email;

                            $phonefirm = $row->phone;
                        }

                        $tasks[] = [
                            'id' => $s->id,
                            'day' => $s->day,
                            'calname' => $s->calname,
                            'terapeutid' => $s->terapeutid,
                            'name' => $s->name,
                            'surname' => $s->surname,
                            'firmid' => $se->id,
                            'email' => $s->email,
                            'emailfirm' => $emailfirm,
                            'phonefirm' => $phonefirm,
                            'phoneuser' => $s->phone
                            ];
                    }
                }
            }
        }
?>

Jak provést takové oštření proti souběhu?

admin@easyweb4u.cz
Backer | 143
+
0
-

Ještě k tomu cronu, testuju to, to zanmená, že to spouštím ručně, tedy 1×. Pokud na cronu, spouští se jednou denně.

admin@easyweb4u.cz
Backer | 143
+
0
-

Ještě poznatek, já mám stejný script (presenter) také na odesílání SMS. Tam se to dělo taky. Původně jsem to odesílání měl v actionDefault(), ale změnil jsem to na public function startup() { parent::startup();… Zdá se, že to na SMSkách pomohlo, zatím se to duplikování neděje, ale na emailech to stále duplikuje.

m.brecher
Generous Backer | 774
+
0
-

@adminaeasyweb4ucz

Jak provést takové ošetření proti souběhu?

Odeslaný email je upomínka uživateli na provedení nějakého úkolu – reminder. Reminder upomíná konkrétní úkol a to asi opakovaně, takže by měl být v tabulce reminder, která má vazbu n : 1 na tabulku calendar, kde jsou úkoly. Upomínky by se měly generovat u nesplněných úkolů nějakým pravidelně spouštěným procesem s vyloučením duplicity ideálně vhodně zvoleným unikátním klíčem ve stavu neodesláno. Další proces by měl emaily upomínek odesílat. V případě úspěšného odeslání označit upomínku jako odeslanou. Pokud by odeslání emailu selhalo, zůstane upomínka označena jako neodeslaná a email se proces pokusí odeslat v dalším kole.

Ty nemáš mezi procesem generujícím upomínky dataEmailExpiredTask() a skutečně odeslanými upomínkami žádnou synchronizaci. Vhodná synchronizace spočívá dle mého názoru v lepším návrhu databáze.

Kamil Valenta
Člen | 765
+
0
-

V access logu uvidíš, kolikrát se to spustilo. Máš to ve startupu, pokud je na stránce nějaký ajax třeba, snadno se to spustí vícekrát za jeden plánovaný request.

Přidej si do calendar sloupec např. „notified“, který bude null. Po vygenerování upozornění v něm bude třeba nějaký unikátní otisk.

foreach ($select as $se) {
    $uni = uniqid();
    $this->database->query("UPDATE calendar SET "
                        . "notified='$uni'"
                        . "WHERE notified IS NULL "
                        . "((day = '$deadline' AND dayend IS NULL) OR (dayend = '$deadline' AND start IS NULL)) "
                        . "AND calendar.inserter = 1 "
                        . "AND calendar.status = 1");
    $sel = $this->database->query("SELECT calendar.*, terapeut.name, terapeut.surname, terapeut.email, terapeut.phone FROM calendar "
                        . "LEFT JOIN terapeut ON terapeut.id = calendar.terapeutid "
                        . "LEFT JOIN users ON users.id = terapeut.userid "
                        . "WHERE users.firmid = $se->id "
                        . "AND calendar.notified = '$uni' ORDER BY day");

Mimo téma:

  1. jaký máš důvod skládat dotazy takto s parametry a nebindovat je tam do placeholderů? (queryArgs() místo query()…)
  2. proč znovu taháš $row = $this->database->table(‚firm‘)->get($se->id); a nesaháš přímo na $se->email a $se->phone?

Editoval Kamil Valenta (28. 7. 2023 10:11)

Kamil Valenta
Člen | 765
+
+2
-

admin@easyweb4u.cz napsal(a):

Původně jsem to odesílání měl v actionDefault(), ale změnil jsem to na public function startup() { parent::startup();… Zdá se, že to na SMSkách pomohlo

To se opravdu jen zdá, ve startup() je ještě o kus větší šance, že se to spustí vícekrát. Ale ani startup, ani action nezaručí, že se to paralelně nespustí.

admin@easyweb4u.cz
Backer | 143
+
0
-

m.brecher napsal(a):

@adminaeasyweb4ucz

Jak provést takové ošetření proti souběhu?

Odeslaný email je upomínka uživateli na provedení nějakého úkolu – reminder. Reminder upomíná konkrétní úkol a to asi opakovaně, takže by měl být v tabulce reminder, která má vazbu n : 1 na tabulku calendar, kde jsou úkoly. Upomínky by se měly generovat u nesplněných úkolů nějakým pravidelně spouštěným procesem s vyloučením duplicity ideálně vhodně zvoleným unikátním klíčem ve stavu neodesláno. Další proces by měl emaily upomínek odesílat. V případě úspěšného odeslání označit upomínku jako odeslanou. Pokud by odeslání emailu selhalo, zůstane upomínka označena jako neodeslaná a email se proces pokusí odeslat v dalším kole.

Ty nemáš mezi procesem generujícím upomínky dataEmailExpiredTask() a skutečně odeslanými upomínkami žádnou synchronizaci. Vhodná synchronizace spočívá dle mého názoru v lepším návrhu databáze.

Díky, to mi přijde jako dobré řešení.

admin@easyweb4u.cz
Backer | 143
+
0
-

Kamil Valenta napsal(a):

V access logu uvidíš, kolikrát se to spustilo. Máš to ve startupu, pokud je na stránce nějaký ajax třeba, snadno se to spustí vícekrát za jeden plánovaný request.

Přidej si do calendar sloupec např. „notified“, který bude null. Po vygenerování upozornění v něm bude třeba nějaký unikátní otisk.

foreach ($select as $se) {
    $uni = uniqid();
    $this->database->query("UPDATE calendar SET "
                        . "notified='$uni'"
                        . "WHERE notified IS NULL "
                        . "((day = '$deadline' AND dayend IS NULL) OR (dayend = '$deadline' AND start IS NULL)) "
                        . "AND calendar.inserter = 1 "
                        . "AND calendar.status = 1");
    $sel = $this->database->query("SELECT calendar.*, terapeut.name, terapeut.surname, terapeut.email, terapeut.phone FROM calendar "
                        . "LEFT JOIN terapeut ON terapeut.id = calendar.terapeutid "
                        . "LEFT JOIN users ON users.id = terapeut.userid "
                        . "WHERE users.firmid = $se->id "
                        . "AND calendar.notified = '$uni' ORDER BY day");

Mimo téma:

  1. jaký máš důvod skládat dotazy takto s parametry a nebindovat je tam do placeholderů? (queryArgs() místo query()…)
  2. proč znovu taháš $row = $this->database->table(‚firm‘)->get($se->id); a nesaháš přímo na $se->email a $se->phone?

Jsou v jiné tabulce – firm, zkusil jsem to nacpat do toho hlavního dotazu LEFT JOIN, ale nefakčilo to. Já si nemyslím, že NENÍ problém v té metodě $this->notice->dataEmailExpiredTask(); to pole se normálně vygeneruje a pak probíhá iterace. Podle mě to dělá ten mailserver.

Editoval admin@easyweb4u.cz (28. 7. 2023 10:38)

Kamil Valenta
Člen | 765
+
0
-

admin@easyweb4u.cz napsal(a):

Jsou v jiné tabulce – firm

Sám se ztrácíš ve svém ne úplně ideálním pojmenování proměnných. $se je přece z firm.

Já si nemyslím, že NENÍ problém v té metodě $this->notice->dataEmailExpiredTask(); to pole se normálně vygeneruje a pak probíhá iterace. Podle mě to dělá ten mailserver.

Vzhledem ke dvěma záporům souhlasím.

nightfish
Člen | 475
+
+1
-

admin@easyweb4u.cz napsal(a):
Podle mě to dělá ten mailserver.

Tím myslíš, že ti mailserver podruhé spustí PHP skript (soudě podle „emaily odeslány 2× včetně zápisu do databáze“)?

Dej si do skriptu nějaké logování a zjistíš, kdo ti jej spouští dvakrát (loguj si ideálně obsah $_SERVER). Nebo se podívej do logu webserveru, jestli v něm uvidíš dva requesty nebo jeden. Pokud jeden, tak je problém spíš v logice tvé aplikace. Pokud dva, tak je problém ve spouštění cronu – v takovém případě se bez popisu, jak cron spouštíš, dále nedostaneme.

Na druhou stranu to nemění nic na tom, že chceš ideální běh cronu zamykat, tak aby současně mohla běžet jenom jedna instance (např. pomocí symfony/lock).

Marek Bartoš
Nette Blogger | 1184
+
0
-
  • Chceš do databáze zapisovat, že už se email odeslal.
    • Odeslání může selhat, z různých důvodů.
    • A při dalším spuštění potřebuješ vědět, že už se mail odeslal
  • Nechceš odesílání spouštět vícekrát současně, je třeba mít zámky
    • Buď pro cron job nebo pro konkrétní email
    • Sám si job můžeš zamknout přes symfony/lock, všechny joby umí zamykat automaticky můj orisai/nette-scheduler (též si tam nastavíš symfony/lock)
    • Konkrétní maily můžeš zamykat zase přes symfony/lock nebo lze použít zamykání přímo v databází – select for update

Dají se použít i message queues – jeden proces do fronty přidá zprávu, že se má odeslat email. Druhý proces zprávu zpracuje (odešle email) a z fronty zprávu odstraní. Je to v mnohém lepší řešení, ale složitější, např. RabbitMQ není easy na konfiguraci.

admin@easyweb4u.cz
Backer | 143
+
0
-

nightfish napsal(a):

admin@easyweb4u.cz napsal(a):
Podle mě to dělá ten mailserver.

Tím myslíš, že ti mailserver podruhé spustí PHP skript (soudě podle „emaily odeslány 2× včetně zápisu do databáze“)?

Dej si do skriptu nějaké logování a zjistíš, kdo ti jej spouští dvakrát (loguj si ideálně obsah $_SERVER). Nebo se podívej do logu webserveru, jestli v něm uvidíš dva requesty nebo jeden. Pokud jeden, tak je problém spíš v logice tvé aplikace. Pokud dva, tak je problém ve spouštění cronu – v takovém případě se bez popisu, jak cron spouštíš, dále nedostaneme.

Na druhou stranu to nemění nic na tom, že chceš ideální běh cronu zamykat, tak aby současně mohla běžet jenom jedna instance (např. pomocí symfony/lock).

To není o cronu, testuju to ručně – spustím tu url, vše proběhne jak má, akorát jsou ty maily zduplikované (ale občas taky ne). Skoro to tak vypadá, že se to spustí 2×. Juknu na ty logy.

admin@easyweb4u.cz
Backer | 143
+
0
-

Kamil Valenta napsal(a):

admin@easyweb4u.cz napsal(a):

Jsou v jiné tabulce – firm

Sám se ztrácíš ve svém ne úplně ideálním pojmenování proměnných. $se je přece z firm.

Já si nemyslím, že NENÍ problém v té metodě $this->notice->dataEmailExpiredTask(); to pole se normálně vygeneruje a pak probíhá iterace. Podle mě to dělá ten mailserver.

Vzhledem ke dvěma záporům souhlasím.

V tom to není, to pole tasks je tak jak má být. Monokrát zkontrolované. Vypadá to, že se script spustí 2×.