Comment 1 Pierre Ossman 2023-09-05 14:09:12 CEST

Normally, Python removes things after being deprecated for two versions. However, it still seems to be in place on Python's master branch as it is still present in the documentation.

Still it is nice to get rid of this warning in our development builds:

> Running /opt/thinlinc/etc/xstartup.d/40-tl-mount-localdrives (Mounting local drives)
> /opt/thinlinc/etc/xstartup.d/40-tl-mount-localdrives:43: DeprecationWarning: > setDaemon() is deprecated, set the daemon attribute instead
>   oOooo0OOO . setDaemon ( True

Comment 2 Alexander Zeijlon 2023-09-14 09:33:31 CEST

When looking at the Thread class in Lib/threading.py, I can see that the daemon property needs to be set before a thread is started, and that an already running thread cannot have its daemon property modified. I.e. A running daemonic thread may not become non-daemonic or vice versa.

This means that setting of the daemon property depends on the running state of the thread, and this is likely why a setter-method, where one can implement error checks, has been used historically.

In Python3, as far back as version 3.0 [1], the Thread class uses @property decorators [2] to implement the error checking directly on the property itself, while the setDaemon-setter only assigns a value to the property. This means that we do not risk bypassing important error checks when we stop using the setDaemon.

[1] https://github.com/python/cpython/blob/v3.0/Lib/threading.py#L665C10-L665C10
[2] https://docs.python.org/3/library/functions.html#property

Comment 4 Alexander Zeijlon 2023-09-14 10:04:45 CEST

All calls to setDaemon in our Python-code have now been replaced with assignment directly to the daemon property.

Comment 5 Alexander Zeijlon 2023-09-14 10:07:36 CEST

A pull request with some of these changes has been sent upstream to Pynfs [1].

[1] http://linux-nfs.org/wiki/index.php/Pynfs

Comment 7 Alexander Zeijlon 2023-09-14 15:50:22 CEST

We found that threading.Event.isSet() has deprecated in favor of threading.Event.is_set()

Will fix this also.

Comment 8 Linn 2023-09-14 16:06:28 CEST

(In reply to Alexander Zeijlon from comment #7)
> We found that threading.Event.isSet() has deprecated in favor of
> threading.Event.is_set()
Saw this message in xinit.log on Fedora 37 (Python 3.11):
> /opt/thinlinc/modules/thinlinc/rpc/rpc.py:151: DeprecationWarning: isSet() is deprecated, use is_set() instead
>  if not self._filled.isSet():

Comment 9 Alexander Zeijlon 2023-09-15 10:34:50 CEST

There are more methods/functions in threading.py that are deprecated and in use in our code. Extending this bug to include these as well.

Comment 10 Alexander Zeijlon 2023-09-15 11:13:15 CEST

The additional threading functions we need to look at are (there are more, but they are not used):
* currentThread(), which is an alias to current_thread()
* Condition.notifyAll(), which is an alias to Condition.notify_all()
* Event.isSet(), which is an alias to Event.is_set()

All three have been aliases to their snake-case counterparts since 2008 [1], and they were deprecated in 2021 [2].


[1] https://github.com/python/cpython/commit/b3085c9e2610943a2c7c038ff7e25639300463fd
[2] https://github.com/python/cpython/commit/9825bdfbd5c966abf1f1b7264992d722a94c9613

It should therefore be safe to stop using the aliases without any risk of bypassing error checks etc.

Comment 12 Alexander Zeijlon 2023-09-15 13:10:59 CEST

Verification:

> MUST:
> * The deprecated methods in threading.py must not be used.
We are now using the actual functions instead of their deprecated aliases.

> * Any applied changes must not introduce regressions in functionality.
These changes were applied only to our test cases and to Pynfs. I have therefore run our unit tests, and I have verified that sharing a local directory still works as expected. Given a file in a shared folder:
* Client-side updates to it become visible in the session.
* Session-side updates to it become visible locally.

> * Applied changes must not increase the risk of introducing faulty behavior.
* For properties, error checks are performed directly on the property and not in the setter-method. See comment 2.
* For aliases, these were only referencing functions. We are now using the referenced function instead of its alias. See comment 10.

> * The new method should be compatible with our supported versions of Python3.
The updated code is compatible with all of our supported versions of Python3. See comment 2 and comment 10.

Comment 13 Linn 2023-09-15 15:50:54 CEST

Tested the acceptance criteria with server build 3382 on Fedora 37 (Py 3.11) and SLES 12 (Py 3.4).

MUST:

  ✅ The deprecated methods in threading.py must not be used.
Checked xinit.log and could no longer see deprecation warnings on Fedora 37 after the changes. On SLES 12, the warnings were never seen due to its lower python version. Also grepped after the old names in the code and did not find any lingering.

  ✅ Applied changes must not introduce regressions in functionality.
Did regression testing on local drives using bug 7232 as a base:
 - Creating a new file
 - Deleting a file
 - Moving a file
 - Non-ascii file name
 - A large file transfers in a reasonable time
 - Permissions for read and write on local drives are followed

  ✅ Applied changes must not increase the risk of introducing faulty behavior.
I agree with the reasoning in comment 2 and comment 10, and I believe this criterion is fulfilled.

  ✅ The new method should be compatible with our supported versions of Python3.
Yes, tested with Python versions on both ends of the spectrum (3.11 vs 3.4) and did not see any issues.


Also checked the code changes, looks good.