Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Prevent restore cache on closing connection #548

Merged
merged 1 commit into from
Sep 5, 2022

Conversation

jakubboucek
Copy link
Contributor

This semantic is wrong:

imap/src/Connection.php

Lines 51 to 56 in c8e26a0

public function close(int $flag = 0): bool
{
$this->resource->clearLastMailboxUsedCache();
return \imap_close($this->resource->getStream(), $flag);
}

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
Copy link

codecov bot commented Sep 1, 2022

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%) ⬆️

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

@Slamdunk
Copy link
Collaborator

Slamdunk commented Sep 5, 2022

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

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

@jakubboucek
Copy link
Contributor Author

jakubboucek commented Sep 5, 2022

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
Copy link
Collaborator

Slamdunk commented Sep 5, 2022

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
Copy link
Contributor Author

jakubboucek commented Sep 5, 2022

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 Slamdunk merged commit 1a0a86e into ddeboer:master Sep 5, 2022
@Slamdunk Slamdunk added the bug label Sep 5, 2022
@Slamdunk
Copy link
Collaborator

Slamdunk commented Sep 5, 2022

Released in 1.14.2, thank you

@jakubboucek
Copy link
Contributor Author

Thx ❤️

saintsloth pushed a commit to saintsloth/imap that referenced this pull request Oct 27, 2022
@jakubboucek jakubboucek deleted the fix-prevent-restore-cache branch February 16, 2023 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants