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.

Sign up for GitHub

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

Jump to bottom

[FIX] Email inbox memory leak on connection failure #26850

Merged
merged 25 commits into from
Oct 14, 2022
Merged

[FIX] Email inbox memory leak on connection failure #26850

merged 25 commits into from
Oct 14, 2022

Conversation

cauefcr
Copy link
Contributor

@cauefcr cauefcr commented Sep 12, 2022 •
edited
Loading

OC-184

Proposed changes (including videos or screenshots)

  • Properly removing event listeners on re-connection.
  • Attachments handling changed from beforeMessageSent to afterMessageSent, as the id of the message was needed, now rocket.cat will send the "Send Attachment To Email" button.
  • Self-disable email inbox when reaching max re-connection attempts

Issue(s)

Steps to test or reproduce

Further comments

@cauefcr cauefcr changed the title [Regression][FIX] Invalid Email Inbox no longer leaks event listeners, and attachments work again [FIX][Regression] Invalid Email Inbox no longer leaks event listeners, and attachments work again Sep 12, 2022
@cauefcr cauefcr changed the title [FIX][Regression] Invalid Email Inbox no longer leaks event listeners, and attachments work again [FIX] Regression: Invalid Email Inbox no longer leaks event listeners, and attachments work again Sep 12, 2022
@cauefcr
OC-184 jira linking
17f1d19
@cauefcr
Merge ssh://github.com/RocketChat/Rocket.Chat into regression/email-m…
76d94d9 
…emory-leak-and-attachment-issues
@cauefcr
forgotten comment
fa26b4b
@cauefcr
Merge branch 'regression/email-memory-leak-and-attachment-issues' of …
f473e6a 
…ssh://github.com/RocketChat/Rocket.Chat into regression/email-memory-leak-and-attachment-issues
@KevLehman KevLehman changed the title [FIX] Regression: Invalid Email Inbox no longer leaks event listeners, and attachments work again [FIX] Email inbox memory leak on connection failure Sep 12, 2022
@codecov
Copy link

codecov bot commented Sep 12, 2022 •
edited
Loading

Codecov Report

Merging #26850 (470b035) into develop (9d42405) will increase coverage by 0.45%.
The diff coverage is n/a.

❗ Current head 470b035 differs from pull request most recent head dae0e56. Consider uploading reports for the commit dae0e56 to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #26850      +/-   ##
===========================================
+ Coverage    40.31%   40.76%   +0.45%     
===========================================
  Files          827      802      -25     
  Lines        18258    17810     -448     
  Branches      2020     1961      -59     
===========================================
- Hits          7360     7261      -99     
+ Misses       10604    10255     -349     
  Partials       294      294
Flag Coverage Δ
e2e 40.76% <ø> (+0.45%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@cauefcr cauefcr marked this pull request as ready for review September 12, 2022 17:59
@cauefcr cauefcr requested a review from a team as a code owner September 12, 2022 17:59
KevLehman
KevLehman requested changes Sep 12, 2022
View reviewed changes
apps/meteor/server/email/IMAPInterceptor.ts Outdated Show resolved Hide resolved
apps/meteor/server/email/IMAPInterceptor.ts Outdated Show resolved Hide resolved
apps/meteor/server/email/IMAPInterceptor.ts Show resolved Hide resolved
apps/meteor/server/features/EmailInbox/EmailInbox_Outgoing.ts Outdated Show resolved Hide resolved
@cauefcr
Copy link
Contributor Author

cauefcr commented Sep 13, 2022

OC-184

@cauefcr cauefcr added this to the 5.2.0 milestone Sep 14, 2022
KevLehman
KevLehman requested changes Sep 14, 2022
View reviewed changes
apps/meteor/server/email/IMAPInterceptor.ts Outdated Show resolved Hide resolved
apps/meteor/server/email/IMAPInterceptor.ts Show resolved Hide resolved
apps/meteor/server/email/IMAPInterceptor.ts Outdated Show resolved Hide resolved
apps/meteor/server/email/IMAPInterceptor.ts Show resolved Hide resolved
apps/meteor/server/email/IMAPInterceptor.ts Outdated Show resolved Hide resolved
apps/meteor/server/features/EmailInbox/EmailInbox_Incoming.ts Show resolved Hide resolved
apps/meteor/server/email/IMAPInterceptor.ts Outdated Show resolved Hide resolved
apps/meteor/server/email/IMAPInterceptor.ts Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Sep 20, 2022

This pull request introduces 1 alert when merging 886be36 into 3023022 - view on LGTM.com

new alerts:

  • 1 for Syntax error

@cauefcr cauefcr removed this from the 5.2.0 milestone Sep 22, 2022
@ggazzo ggazzo added stat: ready to merge PR tested and approved waiting for merge and removed stat: needs QA labels Sep 28, 2022
@cauefcr cauefcr added this to the 5.3.0 milestone Oct 14, 2022
@kodiakhq kodiakhq bot merged commit 11b1588 into develop Oct 14, 2022
@kodiakhq kodiakhq bot deleted the regression/email-memory-leak-and-attachment-issues branch October 14, 2022 20:12
weslley543 pushed a commit that referenced this pull request Oct 17, 2022
@cauefcr @KevLehman @weslley543
[FIX] Email inbox memory leak on connection failure (#26850)
5125a86 
Co-authored-by: Kevin Aleman <11577696+KevLehman@users.noreply.github.com>
@ggazzo ggazzo mentioned this pull request Nov 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KevLehman KevLehman KevLehman approved these changes

Assignees
No one assigned
Labels
squad: omnichannel stat: QA tested stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Milestone
5.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@cauefcr @KevLehman @ggazzo