Skip to content

Commit

Permalink
FIX FAUCardPayment: Rework user abortion, termination and finish_log
Browse files Browse the repository at this point in the history 
In summary, the code that handles the end of a payment is now improved, but still does the same as before. Probably a few bugs were fixed on the way.

Remove the possibility to force-terminate the thread.
Program behavior of QThread terminate() is not well-defined and may leave the program/memory in an undefined state, so we must not allow this.

Remove confusing dialog box about user abortion.

Remove confusing set_finish_log. Make clear that finish_log may only be used outside FAUCardThread.

Rework thread closing.

Remove some unused functions.
  • Loading branch information
@mgmax
mgmax committed Mar 2, 2024
1 parent 7afb165 commit 9156691
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 128 deletions.
36 changes: 0 additions & 36 deletions FabLabKasse/UI/FAUcardPaymentDialogCode.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@ class FAUcardPaymentDialog(QtWidgets.QDialog, Ui_FAUcardPaymentDialog):
# Signal to tell the FAUcardThread to send response acknowledge to the MagnaBox + optional cancel of the process
response_ack = QtCore.Signal(bool)

# Signal to tell the payment handler to terminate the thread
request_termination = QtCore.Signal()

def __init__(self, parent, amount, shopping_backend):
"""
Initializes the FAUcardPaymentDialog. It sets it's member variables and sets up the GUI, including
Expand Down Expand Up @@ -52,9 +49,6 @@ def __init__(self, parent, amount, shopping_backend):
self.counter = 0
self.thread_aborted = False
self.thread_broken = False
self.timer_terminate = QtCore.QTimer()
self.timer_terminate.timeout.connect(self.request_thread_termination)
self.timer_terminate.setSingleShot(True)
self.status = Status.initializing

# Start a timer to periodically update the GUI (show life sign)
Expand Down Expand Up @@ -117,8 +111,6 @@ def update_gui(self, response):
), "Thread response code is not Status or Info!"

self.thread_broken = False
if self.timer_terminate.isActive():
self.timer_terminate.stop()

if isinstance(response[0], Info):
# Abort if checking the last transaction failed
Expand Down Expand Up @@ -240,20 +232,10 @@ def try_reject(self):
If the thread is finished tell the user the process is aborting and process the final abortion in 2 seconds
"""
if self.thread_aborted is not True:
QtWidgets.QMessageBox.warning(
None,
"FAUCard Zahlung",
"Abbrechen gerade nicht möglich.\nBitte warten Sie, bis das Programm an der nächst \
möglichen Stelle abbricht. Beachte bitte, das falls dein Geld schon abgebucht \
wurde, es nicht automatisch rückabgewickelt wird.",
)
self.label_status.setText("Warte auf Möglichkeit zum Abbruch")
if not self.timer_terminate.isActive():
self.timer_terminate.start(60000)
else:
self.label_status.setText("Breche Bezahlung ab.")
self.pushButton_abbrechen.hide()
self.timer_terminate.stop()
QtCore.QTimer.singleShot(2000, self.reject_final)

@QtCore.Slot()
Expand All @@ -262,21 +244,3 @@ def reject_final(self):
Final rejection of the Payment
"""
QtWidgets.QDialog.reject(self)

@QtCore.Slot()
def request_thread_termination(self):
"""
Sets the thread_broken flag to let the user terminate the thread if necessary.
"""
self.thread_broken = True
terminate = QtWidgets.QMessageBox.question(
None,
"FAUCard Zahlung",
"Willst du den Thread terminieren? Wenn du dir nicht sicher bist, antworte mit nein.",
QtWidgets.QMessageBox.Yes,
QtWidgets.QMessageBox.No,
)
if terminate == QtWidgets.QMessageBox.Yes:
logging.error("FAUcardPayment: thread termination was requested")
self.request_termination.emit()
QtCore.QTimer.singleShot(500, self.reject_final)
76 changes: 45 additions & 31 deletions FabLabKasse/faucardPayment/FAUcardPaymentThread.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from qtpy import QtCore, QtWidgets
from decimal import Decimal
from datetime import datetime
import time

from .faucardStates import Status, Info
from .MagPosLog import MagPosLog
Expand Down Expand Up @@ -98,6 +99,7 @@ def __init__(
self.card_number: Optional[int] = None
self.old_balance: Optional[int] = None
self.new_balance: Optional[int] = None
self.run_finished = False

assert amount > 0
self.amount = amount
Expand All @@ -108,7 +110,6 @@ def __init__(
self.ack = False
self.sleep_counter = 0
self.last_sleep = 0
self.should_finish_log = True

self.timestamp_payed: Optional[datetime] = None

Expand Down Expand Up @@ -152,16 +153,6 @@ def set_ack(self, cf: bool) -> None:
self.ack = True
self.cancel = cf

# set if thread should finish logfile with booking entry
@QtCore.Slot(bool)
def set_should_finish_log(self, val: bool) -> None:
"""
Sets the finish logfile flag
:param val: Contains information if the thread should finish the log file
:type val: bool
"""
self.should_finish_log = val

@QtCore.Slot()
def user_abortion(self) -> None:
"""sets cancel flag to cancel the payment"""
Expand Down Expand Up @@ -199,8 +190,7 @@ def sleep(self, seconds: int) -> None:
return

counter = self.sleep_counter
# BUG? what happens if sleep_counter accidentally (timing glitch, ...) becomes larger than seconds*10?
while self.sleep_counter is not seconds * 10:
while self.sleep_counter < seconds * 10:
if self.cancel == True:
raise self.UserAbortionError("sleep")
if counter == self.sleep_counter:
Expand Down Expand Up @@ -234,13 +224,33 @@ def quit(self) -> None:

@QtCore.Slot()
def run(self) -> None:
"""
Run payment. See _run().
This function blocks until the payment has finished or an Exception was thrown.
Afterwards, run_finished is set to True.
This function may call the event-loop during the payment operation ("voluntary preemption").
"""
try:
self._run()
finally:
# Make sure that in all cases,
try:
# - the status is written to the database.
if self.log is not None:
self.log.set_status(self.status, self.info)
finally:
# - and run_finished is set to True
self.run_finished = True

def _run(self) -> None:
"""
Billing routine for FAU-Card payment. Runs following steps:
1. Check last transaction result
2. Initialize log class and connection to MagnaBox
3. Read the card the user puts on the reader
4. Decrease the amount from card balance
(Optional: 5. finish log file)
(Note: The MagPosLog log entry is NOT marked as "booking done" here, because at the end of this function the booking in the cash register is not yet done. After the booking in the cash register was done, faucardPayment.faucard.finish_log() needs to be called to do that.)
After each step the routine waits till the GUI sends a positive feedback by set_ack slot.
The GUI can send a cancel flag with the response_ack signal and is able to quit the process after every step.
"""
Expand Down Expand Up @@ -290,10 +300,6 @@ def run(self) -> None:
self.log.set_timestamp_payed(self.timestamp_payed)
logging.debug("FAUcardThread: New Balance: {}".format(self.new_balance))

# 5. finish log entry
if self.should_finish_log is True:
self.finish_log()

except (magpos.ResponseError, self.ConnectionError) as e:
logging.error("FAUcardThread: {}".format(e))
self.terminate()
Expand Down Expand Up @@ -496,18 +502,21 @@ def _read_card(
def _decrease_balance(self) -> int:
"""
Decreases card balance by self.amount_cents by following steps:
0. If user aborted: Quit by raising UserAbortionError.
1. Try to decrease balance
-> a: Successful
2. Continue with step 5
-> b: Connection was lost?
Continue with step 5
-> aa: Payment was not executed (user removed card)
Continue with step 0.
-> b: Connection was lost (i.e., we don't know yet if the payment was successful)
2. Try to reestablish connection, ignore connection relevant errors
3. Check if the payment you tried has been successfully executed
-> a: was successfully executed:
4. Continue with step 5
-> b: the payment was not executed:
4: Go back to step 1
4: Go back to step 0
-> c: it is unclear if the payment was executed or not:
4. Quit the process by raising an Exception
4. Quit by raising an Exception
5. Check if payment was correct and log it
:return: New balance on success. Otherwise an exception will be raised.
:rtype: int
Expand All @@ -533,26 +542,28 @@ def _decrease_balance(self) -> int:

decrease_result = None

# 1. Try to decrease balance
# 0./1. Check for user abortion and try to decrease balance
while retry:
retry = False
self.set_cancel_button_enabled.emit(
True
) # User must be able to abort if he decides to
try:
logging.debug("FAUcard: 0. Check if user aborted")
self.check_user_abort(
"decreasing balance: User Aborted"
) # Will only be executed if decrease command has not yet been executed
logging.debug("FAUcard: Trying to decrease balance")
logging.debug("FAUcard: 1. Trying to decrease balance")
decrease_result = self.pos.decrease_card_balance_and_token(
self.amount_cents, self.card_number
)

# Catch ResponseError if not Card on Reader and retry
except magpos.ResponseError as e:
if e.code is magpos.codes.NO_CARD:
logging.info("FAUcard: No card, retrying...")
logging.info("FAUcard: No card, retrying... (1.aa)")
self.pos.response_ack()
# -> Back to 0.
retry = True
continue
else:
Expand All @@ -569,11 +580,11 @@ def _decrease_balance(self) -> int:
self.response_ready.emit([Info.con_error])
lost = True

# Abortion of the process not allowed
# self.set_cancel_button_enabled.emit(False)
# Clear cancel Flag if user tried to abort: no abortion allowed after this tep
QtCore.QCoreApplication.processEvents()
self.cancel = False
# From now on (all lines in this function below this comment), abortion of the process not allowed.
# The only exception is when we are sure that the payment did not succeed (the amount was not deducted from the card), in which case we can go back to step 0 and retry the payment.
#
# "Abortion not allowed" is implemented by NOT calling _wait_for_ack() of check_user_abort() in any line below this comment.
# The user can still click on the cancel button and therefore set self.cancel=True, but this will be ignored until we get to a point at which canceling is allowed.

# if connection lost (1.b)
if lost:
Expand All @@ -582,7 +593,7 @@ def _decrease_balance(self) -> int:
while lost:
lost = False

logging.debug("FAUcard: 2 Try to reastablish connect")
logging.debug("FAUcard: 2 Try to reestablish connect")
try:
# release serial port
self.pos.close()
Expand All @@ -605,6 +616,9 @@ def _decrease_balance(self) -> int:
IOError,
):
lost = True
# wait a short time to avoid flooding the harddrive with error messages when the connection is permanently lost
# we don't use self.sleep() here because we do NOT want to process cancel-events
time.sleep(10)

self.info = Info.con_back
self.response_ready.emit([Info.con_back])
Expand Down
91 changes: 32 additions & 59 deletions FabLabKasse/faucardPayment/faucard.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from datetime import datetime
from decimal import Decimal
from qtpy import QtCore, QtWidgets, QtGui
import logging

from .MagPosLog import MagPosLog
from ..UI.FAUcardPaymentDialogCode import FAUcardPaymentDialog
Expand Down Expand Up @@ -39,32 +40,52 @@ def __init__(self, parent, amount, shopping_backend):
self.dialog = FAUcardPaymentDialog(
parent=parent, amount=self.amount, shopping_backend=self.shopping_backend
)
self.dialog.request_termination.connect(
self.threadTerminationRequested, type=QtCore.Qt.DirectConnection
)
self.worker = FAUcardThread(
dialog=self.dialog, amount=self.amount, thread=self.thread
)

def executePayment(self):
"""
Starts the Payment-process and Dialog
Runs the Payment-process: Open dialog, wait for payment to complete (or to fail), close dialog, clean up.
Expected usage when the user requests "Start FAUCard payment":
1. Call this function
2. If it returned True:
The payment has succeeded.
a. Write the corresponding booking into the accounting / cash register database.
b. call finish_log(), which then notes in the FAUCard MagPosLog logging that the payment has been fully booked.
Note that if a) crashes, then we can see this in the MagPos database status.
Else:
The payment has failed, go back to the shopping-cart view so that the user can restart the payment.
:return: True on success, False otherwise
:rtype: bool
"""
# Start the process
self.thread.start()
self.dialog.show()

# Execute the GUI
# Execute the GUI and wait until the dialog has closed.
success = self.dialog.exec_()

if success == QtWidgets.QDialog.Accepted:
return True
else:
# Wait for thread to finish cleanup
self.thread.wait(1000)
return False
# Now, the payment routine (FAUCardThread.run()) has returned (or raised an Exception).
# double-check this:
while not self.worker.run_finished:
# TODO: rewrite the code so that it is obvious that this can not happen.
# --> FAUCardThread itself should subclass QThread and override run().
logging.error(
"Payment routine did not finish (deadlock?), waiting... This should never happen."
)
time.sleep(10)

# Stop thread's event loop, it is no longer needed
self.thread.quit()
while not self.thread.wait(10000):
logging.error(
"thread refuses to stop, waiting... This should never happen except when e.g. the system is under extreme CPU load."
)

return success == QtWidgets.QDialog.Accepted

def getPaidAmount(self):
"""
Expand All @@ -83,54 +104,6 @@ def getWantReceipt(self):
# always output a receipt due to legal requirements
return True

def finishLogEntry(self):
"""
Completes the log entry in the MagPosLog and closes all open threads and dialogs
"""
if self.thread.isRunning():
QtCore.QMetaObject.invokeMethod(
self.worker,
"set_ack",
QtCore.Qt.QueuedConnection,
QtCore.Q_ARG(bool, False),
)
self.close()

@QtCore.Slot()
def threadTerminationRequested(self):
"""
Terminates the self.thread if requested.
"""
self.thread.wait(100)
self.thread.quit()
if not self.thread.wait(1000):
self.thread.terminate()

def close(self):
"""
Quits self.thread, closes self.dialog
"""
self.dialog.close()
if self.thread.isRunning():
QtCore.QMetaObject.invokeMethod(
self.worker,
"set_should_finish_log",
QtCore.Qt.QueuedConnection,
QtCore.Q_ARG(bool, False),
)
QtCore.QMetaObject.invokeMethod(
self.worker,
"set_ack",
QtCore.Qt.QueuedConnection,
QtCore.Q_ARG(bool, False),
)
self.thread.wait(100)
if not self.thread.isRunning():
return
self.thread.quit()
if not self.thread.wait(1000):
self.thread.terminate()


def check_last_transaction():
"""
Expand Down
2 changes: 0 additions & 2 deletions FabLabKasse/shopping/payment_methods.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -353,8 +353,6 @@ def _show_dialog(self):
self.amount_paid = pay_func.getPaidAmount()
self.amount_returned = 0

pay_func.close()

def _end_of_payment(self):
"""
Is required to complete the MagPosLog logging after the payment routine.
Expand Down

0 comments on commit 9156691

Please sign in to comment.