SAP / PyRFC

Asynchronous, non-blocking SAP NW RFC SDK bindings for Python
http://sap.github.io/PyRFC
Apache License 2.0
500 stars 132 forks source link

timeout halting main thread exit #352

Closed muellma-items closed 8 months ago

muellma-items commented 8 months ago

Hello, first of all thanks so much for this library. I have been using it in small projects for years now and we just started implementing it in a bigger project.

Describe the bug When we use the timeout property on a connection or an RFC call and we have an error during the RFC call then the produced Timer object will not be cancelled. This leads to unexpected behaviour. If the main thread tries to exit, it will wait on all non-daemon threads. As a Timer is not a daemon, it will then proceed to wait before finally finishing.

To Reproduce Run this script. Uncomment the lower part for more information

import pyrfc
import threading

# we build a connection with 15 seconds timeout. As far as I understand it, it
# should cancel requests that take longer than 15 seconds but if it returns or
# has an error before then the timeout should do nothing. However, if we set timeout
# here
rfcconn = pyrfc.Connection(
    config={"timeout": 15},
    ashost="yourhost",
    sysnr="00",
    client="100",
    user="me",
    passwd="secret",
)
# and then call a function module with an ERROR like wrong OPTION like "REQUTEXXXXT"
try:
    rfcconn.call("STFC_CONNECTION", REQUTEXXXXT="Hello SAP!")
except Exception as e:
    print(f"Error encountered during RFC call: {e}")

print("Script should end NOW. However, we wait another ~14 seconds")
# then if the scripts main thread returns it still waits on the timer to do its thing. So
# essentially the whole script is halted for another ~14s

# # as we can see there is still a thread running for the timer
# print(threading.enumerate())
# timer_thread = threading.enumerate()[1]
# print(timer_thread)

# # When we cancel the timer, the script exits as expected
# timer_thread.cancel()
# print("Now we really quit as expected!")

Environment

Additional context Troubleshooting this issue has taken me some time and I have read quite a bit about threading. The way I see it either the Timer should be cancelled when there is any error during the Connection.call() method or Connection.close() should kill all timers it can find that belong to it, possibly using named Timers. If possible the Timer should never be left alive after the Connection.call() method has finished.

muellma-items commented 8 months ago

PR with fix was merged