davidmoten / subethasmtp

SubEtha SMTP is a Java library for receiving SMTP mail
Other
149 stars 40 forks source link

Incapsulate Session.getRawInput() to prevent close Socket#InputStream #113

Open valenpo opened 1 year ago

valenpo commented 1 year ago

All InputStream's that decorate Socket.inputStream could be used in try block safe. Close of Socket InputStream happens on Session class.

codecov[bot] commented 1 year ago

Codecov Report

Attention: Patch coverage is 96.29630% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 71.02%. Comparing base (e4c24f4) to head (8673475). Report is 19 commits behind head on master.

:exclamation: Current head 8673475 differs from pull request most recent head 87c94b8

Please upload reports for the commit 87c94b8 to get more accurate results.

Files Patch % Lines
...subethamail/smtp/internal/command/DataCommand.java 88.88% 0 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #113 +/- ## ============================================ + Coverage 70.92% 71.02% +0.09% - Complexity 463 466 +3 ============================================ Files 72 72 Lines 2373 2381 +8 Branches 248 248 ============================================ + Hits 1683 1691 +8 Misses 572 572 Partials 118 118 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

davidmoten commented 11 months ago

excuse the delay, what do we gain from this change? Do we have a resource leak? I don't believe we do so I don't see any benefit (reduces false positives from code quality checkers perhaps?).

valenpo commented 11 months ago

excuse the delay, what do we gain from this change? Do we have a resource leak? I don't believe we do so I don't see any benefit (reduces false positives from code quality checkers perhaps?).

we encapsulate the stream in the place where it should be closed. Otherwise, any other user who uses the recommended try catch block will fall into the trap that I fell into.

We are also correcting the TODO, which says that the stream should be closed in the place from where we open it. In the current implementation, it is necessary to keep in mind where it is necessary and where it is not necessary to close the streams, this is not very nice and correct. And an open door for future mistakes.

valenpo commented 11 months ago

@davidmoten Hi. Same way tika incapsulate stream that should not be closed. Any update?

davidmoten commented 11 months ago

Sorry Valentin, working two jobs at the moment and am short on headspace. What I plan to do is look at your suggestion in the IDE and give you some feedback then.

valenpo commented 11 months ago

Thanks!

On 18 Oct 2023, at 10:38, Dave Moten @.***> wrote:

Sorry Valentin, working two jobs at the moment and am short on headspace. What I plan to do is look at your suggestion in the IDE and give you some feedback then.

— Reply to this email directly, view it on GitHub https://github.com/davidmoten/subethasmtp/pull/113#issuecomment-1767852753, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIMLQ77HVSIFFWBOYCDGH3X76BO5AVCNFSM6AAAAAA5QTYA7SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRXHA2TENZVGM. You are receiving this because you authored the thread.

Regards, Valentin Popov

davidmoten commented 11 months ago

Can you drop the changes to the pom please (jacoco and guava-mini)? And rename convert to read?

valenpo commented 11 months ago

Done, sorry for delay.

On 18 Oct 2023, at 10:42, Dave Moten @.***> wrote:

Can you drop the changes to the pom please (jacoco and guava-mini)? And rename convert to read?

— Reply to this email directly, view it on GitHub https://github.com/davidmoten/subethasmtp/pull/113#issuecomment-1767862757, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIMLQYBMGQT7IDPIJYL7UTX76B5BAVCNFSM6AAAAAA5QTYA7SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRXHA3DENZVG4. You are receiving this because you authored the thread.

Regards, Valentin Popov

valenpo commented 10 months ago

@davidmoten hi. Any chance to look?