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

[plugin] fix: dispose deploy listener on close #6127

Merged
merged 1 commit into from
Sep 18, 2019

Conversation

ensorrow
Copy link
Contributor

@ensorrow ensorrow commented Sep 6, 2019

What it does

image
The deploy listener won't dispose when connection got disposed, which will cause memory leak.

How to test

  1. Lower the max listener count in theia/packages/core/src/common/event.ts
  2. trying to disconnect the client (you can use the proxy below to simulate the disposed connection)
var express = require('express');
var app = express();
var httpProxy = require('http-proxy');
var proxy = httpProxy.createProxyServer({ target: 'http://localhost:3000', ws: true });

var server = require('http').createServer(app);

// proxy HTTP GET / POST
app.get('/*', function (req, res) {
  console.log("proxying GET request", req.url);
  proxy.web(req, res, {});
});
app.post('/*/*', function (req, res) {
  console.log("proxying POST request", req.url);
  proxy.web(req, res, {});
});

// Proxy websockets
server.on('upgrade', function (req, socket, head) {
  console.log("proxying upgrade request", req.url);
  proxy.ws(req, socket, head);
});

server.listen(3001);

Review checklist

Reminder for reviewers

@ensorrow ensorrow requested a review from a team as a code owner September 6, 2019 10:12
@akosyakov
Copy link
Member

@ensorrow please sign your work and accept ECA, otherwise we cannot continue

@akosyakov akosyakov added the plug-in system issues related to the plug-in system label Sep 6, 2019
@ensorrow
Copy link
Contributor Author

@ensorrow please sign your work and accept ECA, otherwise we cannot continue

Done

@akosyakov
Copy link
Member

@ensorrow thank you @marcdumais-work i am not sure why eca check is still not happy

Maybe an empty line is required before sign-off line.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes looks good and make sense to me.

@benoitf
Copy link
Contributor

benoitf commented Sep 10, 2019

@ensorrow

the Signed-Off and the commit e-mail should be the same

Author: xunhe.lzy <.....@alibaba-inc.com>
Signed-off-by: Louis Lv <.....@qq.com>

@akosyakov
Copy link
Member

akosyakov commented Sep 12, 2019

eca is still not happy :( cc @marcdumais-work

Signed-off-by: ensorrow <1121264060@qq.com>
@ensorrow
Copy link
Contributor Author

Too busy to fix eca problem in time, sorry for that @akosyakov

@akosyakov
Copy link
Member

@ensorrow thank you for dealing with ECA check :)

@akosyakov akosyakov merged commit 4991c24 into eclipse-theia:master Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plug-in system issues related to the plug-in system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants