Obsoleted Close() methods

[WenningQiu]

The Close() method on the follow types are marked with [Obsolete("Dispose any opened Stream directly")]. However, replacing the Close() calls with directly disposing the open stream does not appear to work (the decryption of the encrypted data fails with a variety of errors). Each of those Close() calls has substantial logic which I doubt can simply be replaced with directly disposing the open streams. I want to make sure if my understanding of the Obsolete directive is incorrect or if the Obsolete attribute should be removed.

• PgpLiteralDataGenerator
• PgpCompressedDataGenerator
• PgpEncryptedDataGenerator

[peterdettman]

You'll have to give an example of something that doesn't work if you switch from calling close on the generator to disposing the Stream returned from the generator's Open method(s).

All our tests and several utilities are using the disposing approach without issue. Indeed all those Open methods are actually returning a WrappedGeneratorStream, whose Dispose just calls the generator's Close, so there's not much room for confusion.

[WenningQiu]

@peterdettman Thanks for looking into this. Here's the method that has the deprecated Close() commented out and would fail our automated tests.

        internal void EncryptAndSign(Stream inputStream, Stream outputStream, bool mustSign, bool armor = true, string fileName = "")
        {
            if (mustSign && _keyPair.PgpSecretKey is null)
                throw new Exceptions.EncryptionException("Cannot sign the encrypted message because no private key is provided");    

            try
            {
                if (armor)
                    outputStream = new ArmoredOutputStream(outputStream);

                // Init encrypted data generator
                var encryptedDataGenerator = new PgpEncryptedDataGenerator(SymmetricKeyAlgorithmTag.TripleDes, new SecureRandom());
                encryptedDataGenerator.AddMethod(_keyPair.PgpPublicKey);
                using var encryptedOut = encryptedDataGenerator.Open(outputStream, new byte[BufferSize]);

                // Init compression
                var compressedDataGenerator = new PgpCompressedDataGenerator(CompressionAlgorithmTag.Zip);
                using var compressedOut = compressedDataGenerator.Open(encryptedOut);

                // Init signature
                var signatureGenerator = (_keyPair.PgpSecretKey is null) ? null : new PgpSignatureGenerator(_keyPair.PgpSecretKey.PublicKey.Algorithm, HashAlgorithmTag.Sha256);

                if (signatureGenerator is not null)
                {
                    signatureGenerator.InitSign(PgpSignature.BinaryDocument, _keyPair.PgpPrivateKey);
                    foreach (string userId in _keyPair.PgpSecretKey.PublicKey.GetUserIds())
                    {
                        var spGen = new PgpSignatureSubpacketGenerator();
                        spGen.AddSignerUserId(false, userId);
                        signatureGenerator.SetHashedSubpackets(spGen.Generate());
                        // Just the first one!
                        break;
                    }
                    signatureGenerator.GenerateOnePassVersion(false).Encode(compressedOut);
                }

                // Create the Literal Data generator output stream
                var literalDataGenerator = new PgpLiteralDataGenerator();

                using var literalOut = literalDataGenerator.Open(compressedOut, PgpLiteralData.Binary, fileName,
                                                              SystemDateTime.UtcNow, new byte[BufferSize]);

                byte[] buf = new byte[BufferSize];
                int len;
                while ((len = inputStream.Read(buf, 0, buf.Length)) > 0)
                {
                    literalOut.Write(buf, 0, len);

                    if (signatureGenerator is not null)
                        signatureGenerator.Update(buf, 0, len);
                }

                //https://github.com/bcgit/bc-csharp/issues/455
                //literalDataGenerator.Close(); //does not implement IDisposable

                if (signatureGenerator is not null)
                    signatureGenerator.Generate().Encode(compressedOut);

                //compressedDataGenerator.Close(); //does not implement IDisposable

                //encryptedDataGenerator.Close();//does not implement IDisposable

                inputStream.Close();

                if (armor)
                    outputStream.Close();
            }
            catch (Exception e)
            {
                Logger.Warning(LogCategory.PGP, "e8a6125c-e13b-4f21-9846-5865dc3b0acc", null, e, "Error during data encryption");
                throw new EncryptionException("PGP encryption failed");
            }
        }

Below is the diff of code changes:

[peterdettman]

I guess the problem is that in the changed code the Dispose is happening at the end of the try block, which is after the Close of inputStream and outputStream has happened. I'd suggest making explicit (nested) scopes for the using declarations, or at least for the declaration of encryptedOut (with the scope closing before the inputStream.Close()).