ddeboer / imap

Object-oriented, fully tested PHP IMAP library
MIT License
889 stars 253 forks source link

Fix: Prevent restore cache on closing connection #548

Closed jakubboucek closed 2 years ago

jakubboucek commented 2 years ago

This semantic is wrong:

https://github.com/ddeboer/imap/blob/c8e26a01ba4367fc4ae9536633c462d92cbca8d8/src/Connection.php#L51-L56

because calling of $this->resource->getStream() is refill cache cleared on previous line.

This PR is change call order to prevent refill cache with reference to closed resource.

codecov[bot] commented 2 years ago

Codecov Report

Merging #548 (f41a047) into master (650e43d) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master     #548   +/-   ##
=========================================
  Coverage     95.58%   95.58%           
  Complexity      357      357           
=========================================
  Files            45       45           
  Lines           838      839    +1     
=========================================
+ Hits            801      802    +1     
  Misses           37       37           
Impacted Files Coverage Δ
src/Connection.php 88.73% <100.00%> (+0.16%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Slamdunk commented 2 years ago

because calling of $this->resource->getStream() is refill cache cleared on previous line.

That's intended: what's your issue about this?

jakubboucek commented 2 years ago

Sometime is occured error when connection to server is interrupted or server go down for unknown reason. Then we call $imap->close() to release resouces on client's side.

This bug causes to call imap_reopen() during closing. That does not make sense and it's consume huge time to connect with dead server and throwing confusing exception when it fail again.

This PR is changing order of call clearLastMailboxUsedCache() and resource->getStream() methods to prevent re-open connection to dead server when client obvious want to close it.

The $this->resource->getStream(); call does NOT cause to re-open connection when cache is filled because ImapResource is trusting to cache and not checking if connections is alive (that's good for performance).

Slamdunk commented 2 years ago

That does not make sense and it's consume huge time to connect with dead server and throwing confusing exception when it fail again.

This PR still raises an Exception when this happens, doesn't it?

jakubboucek commented 2 years ago

Example of bug causes:

  1. Client is connected to server.
  2. Server becomes offline.
  3. Client throws the Exception.
  4. I have try/catch, I handling Exception, here I call $client->close() to release resources.
  5. Here is library trying to connect to offline server.
  6. It cashes AGAIN, it AGAIN thow Exception, it is NOT catched by try/catch because still handling previous exception.
  7. This Exception is confusing, because is throwed when connection want to close.

When I changed code like this PR, it does not throw two Exception, only one, which is handled.

Slamdunk commented 2 years ago

Released in 1.14.2, thank you

jakubboucek commented 2 years ago

Thx ❤️