Repository: dmwm/wmcore
Issue: [py2py3] pickle load/dump should always read bytes

**Impact of the bug**
WMCore in general

**Describe the bug**
`pickle.dump(object, file)` and `pickle.load(file)` require `file` to be  of type bytes and we do not enforce it everywhere in our code yet. The list of potential problematic usage of pickle is at [1]

**How to reproduce it**
We have not any specific bugs, i just looked for `pickle` in our code and tried to spot problematic uses.

**Expected behavior**
We should always pass bytes to pickle.

**Additional context and error message**

[1]

https://github.com/dmwm/WMCore/blob/2b41a0cc1fc635c71ce790b6d2433d3b31da7304/src/python/PSetTweaks/PSetTweak.py#L370-L371

https://github.com/dmwm/WMCore/blob/2b41a0cc1fc635c71ce790b6d2433d3b31da7304/src/python/PSetTweaks/PSetTweak.py#L399-L400

https://github.com/dmwm/WMCore/blob/2b41a0cc1fc635c71ce790b6d2433d3b31da7304/src/python/WMCore/BossAir/Plugins/MockPlugin.py#L40-L42

https://github.com/dmwm/WMCore/blob/2b41a0cc1fc635c71ce790b6d2433d3b31da7304/src/python/WMCore/BossAir/Plugins/MockPlugin.py#L167-L173

Here we should also fix how we interface with `base64`, since `base64.encodestring()` `base64.decodestring()` are not available in py3
https://github.com/dmwm/WMCore/blob/2b41a0cc1fc635c71ce790b6d2433d3b31da7304/src/python/WMCore/ThreadPool/ThreadPool.py#L147-L149

https://github.com/dmwm/WMCore/blob/2b41a0cc1fc635c71ce790b6d2433d3b31da7304/src/python/WMCore/ThreadPool/ThreadSlave.py#L176

https://github.com/dmwm/WMCore/blob/2b41a0cc1fc635c71ce790b6d2433d3b31da7304/test/python/WMCore_t/BossAir_t/BossAir_t.py#L318-L319

https://github.com/dmwm/WMCore/blob/74bd93ca5ebd50c7b84e72223074ef7945dfce21/test/python/WMCore_t/Misc_t/WMAgent_t.py#L448-L450






Changed files:
src/python/WMCore/BossAir/Plugins/MockPlugin.py, src/python/PSetTweaks/PSetTweak.py, test/python/WMCore_t/BossAir_t/BossAir_t.py, src/python/WMCore/ThreadPool/ThreadPool.py, src/python/WMCore/ThreadPool/ThreadSlave.py, test/python/WMCore_t/Misc_t/WMAgent_t.py

diff --git a/src/python/PSetTweaks/PSetTweak.py b/src/python/PSetTweaks/PSetTweak.py
index 887af4b84..16d292e3a 100644
--- a/src/python/PSetTweaks/PSetTweak.py
+++ b/src/python/PSetTweaks/PSetTweak.py
@@ -367,7 +367,7 @@ class PSetTweak(object):
             with open(filename, "w") as handle:
                 handle.write(self.jsonise())
         if formatting == "pickle":
-            with open(filename, "w") as handle:
+            with open(filename, "wb") as handle:
                 pickle.dump(self, handle)
         if formatting == "simplejson":
             with open(filename, "w") as handle:
@@ -396,7 +396,7 @@ class PSetTweak(object):
             raise RuntimeError(msg)
 
         if formatting == "pickle":
-            with open(filename, 'r') as handle:
+            with open(filename, 'rb') as handle:
                 unpickle = pickle.load(handle)
             self.process.__dict__.update(unpickle.__dict__)
 
diff --git a/src/python/WMCore/BossAir/Plugins/MockPlugin.py b/src/python/WMCore/BossAir/Plugins/MockPlugin.py
index 058f87d23..3bf6c3677 100644
--- a/src/python/WMCore/BossAir/Plugins/MockPlugin.py
+++ b/src/python/WMCore/BossAir/Plugins/MockPlugin.py
@@ -37,7 +37,7 @@ def processWorker(myinput, tmp):
             if jj['cache_dir'].count("Production/LogCollect") > 0:
                 if lcreport is not None:
                     lcreport.task = "/" + taskName + "/Production/LogCollect"
-                    with open(outfile, 'w') as f:
+                    with open(outfile, 'wb') as f:
                         logging.debug('Process worker is dumping the LogCollect report to ' + f.name)
                         pickle.dump(lcreport, f)
                     continue
@@ -164,12 +164,12 @@ class MockPlugin(BasePlugin):
             self.start( self.myinput )
 
         #for each job we will need to modify the default Report (the output of each job).
-        with open(self.fakeReport) as f:
+        with open(self.fakeReport, "rb") as f:
             report = pickle.load(f)
 
         lcreport = getattr(self.config.BossAir.MockPlugin, 'lcFakeReport', None)
         if lcreport != None:
-            with open(lcreport) as f:
+            with open(lcreport, "rb") as f:
                 lcreport = pickle.load(f)
 
         for jj in jobs:
diff --git a/src/python/WMCore/ThreadPool/ThreadPool.py b/src/python/WMCore/ThreadPool/ThreadPool.py
index 05789722d..c46a3aa96 100644
--- a/src/python/WMCore/ThreadPool/ThreadPool.py
+++ b/src/python/WMCore/ThreadPool/ThreadPool.py
@@ -18,6 +18,7 @@ import logging
 import random
 import threading
 import time
+from Utils.Utilities import encodeUnicodeToBytes
 try:
     import cPickle as pickle
 except ImportError:
@@ -26,6 +27,8 @@ except ImportError:
 from WMCore.ThreadPool.WorkQueue import ThreadPool as Queue
 from WMCore.WMFactory import WMFactory
 
+from Utils.PythonVersion import PY3
+
 class ThreadPool(Queue):
     """
     _ThreadPool_
@@ -144,9 +147,10 @@ class ThreadPool(Queue):
 
         """
         self.lock.acquire()
-        args = {'event': str(key), \\
-                'component' : self.component.config.Agent.componentName, \\
-                'payload' : base64.encodestring(pickle.dumps(parameters)), \\
+        base64_encoder = base64.encodebytes if PY3 else base64.encodestring
+        args = {'event': str(key),
+                'component' : self.component.config.Agent.componentName,
+                'payload' : base64_encoder(encodeUnicodeToBytes(pickle.dumps(parameters))),
                 'thread_pool_id' : self.threadPoolId}
         myThread = threading.currentThread()
         myThread.transaction.begin()
diff --git a/src/python/WMCore/ThreadPool/ThreadSlave.py b/src/python/WMCore/ThreadPool/ThreadSlave.py
index ddbe176fc..b6972dca8 100644
--- a/src/python/WMCore/ThreadPool/ThreadSlave.py
+++ b/src/python/WMCore/ThreadPool/ThreadSlave.py
@@ -32,6 +32,9 @@ except ImportError:
 from WMCore.Database.Transaction import Transaction
 from WMCore.WMFactory import WMFactory
 
+from Utils.PythonVersion import PY3
+from Utils.Utilities import encodeUnicodeToBytes
+
 class ThreadSlave(object):
     """
     __ThreadSlave__
@@ -173,7 +176,8 @@ class ThreadSlave(object):
         # we commit here because if the component crashes this is where
         # if will look for lost threads (the ones that are in the process state
         myThread.transaction.commit()
-        return (result[1], pickle.loads(base64.decodestring(result[2])))
+        base64_decoder = base64.decodebytes if PY3 else base64.decodestring
+        return (result[1], pickle.loads(base64_decoder(encodeUnicodeToBytes(result[2]))))
 
     def removeWork(self):
         """
diff --git a/test/python/WMCore_t/BossAir_t/BossAir_t.py b/test/python/WMCore_t/BossAir_t/BossAir_t.py
index 7f7206aee..a629b28a2 100644
--- a/test/python/WMCore_t/BossAir_t/BossAir_t.py
+++ b/test/python/WMCore_t/BossAir_t/BossAir_t.py
@@ -315,9 +315,8 @@ class BossAirTest(unittest.TestCase):
             testJob['cache_dir'] = jobCache
             testJob.save()
             jobGroup.add(testJob)
-            output = open(os.path.join(jobCache, 'job.pkl'), 'w')
-            pickle.dump(testJob, output)
-            output.close()
+            with open(os.path.join(jobCache, 'job.pkl'), 'wb') as output:
+                pickle.dump(testJob, output)
 
         return testJob, testFile
 
diff --git a/test/python/WMCore_t/Misc_t/WMAgent_t.py b/test/python/WMCore_t/Misc_t/WMAgent_t.py
index b7988b47f..4507840ae 100644
--- a/test/python/WMCore_t/Misc_t/WMAgent_t.py
+++ b/test/python/WMCore_t/Misc_t/WMAgent_t.py
@@ -445,9 +445,8 @@ class WMAgentTest(unittest.TestCase):
         self.assertTrue('job_1' in os.listdir(groupDirectory))
         jobFile = os.path.join(groupDirectory, 'job_1', 'job.pkl')
         self.assertTrue(os.path.isfile(jobFile))
-        f = open(jobFile, 'r')
-        job = pickle.load(f)
-        f.close()
+        with open(jobFile, 'rb') as f:
+            job = pickle.load(f)
 
 
         self.assertEqual(job['workflow'], name)