Clean-up servers when closing LiveDevelopment #10453

Merged
merged 4 commits into from May 14, 2015

Projects

None yet

3 participants

@sebaslv
Contributor
sebaslv commented Jan 23, 2015

Fix #10374 LiveDevMultiBrowser fails to connect when a local server is configured.

Servers registered by LiveDevelopment are now being cleaned-up when closing the session since LiveDevServerManager retrieves an instance of UserServer to LiveDevMultiBrowser in this case.
In order to be able to remove registered servers, a new LiveDevServerManager.removeServer(provider) operation was added and LiveDevServerManager.registerServer() now returns an object handler to be used for removing it. Registrations of servers at LiveDevelopment were moved from init() to open() and clean-up of servers take place at LiveDevelopment.close().

@sebaslv sebaslv Clean-up servers when closing LiveDevelopment
Fix for #10374. LiveDevMultiBrowser can't load when a local server is configured.
Servers registered by LiveDevelopment are now being cleaned-up when closing the
session.
c747b13
@nethip
Contributor
nethip commented Apr 3, 2015

@busykai Would you mind taking a look at this PR?

@busykai
Member
busykai commented Apr 3, 2015

@nethip, sure! will do (next week, most likely).

@busykai busykai commented on an outdated diff Apr 19, 2015
src/LiveDevelopment/LiveDevelopment.js
@@ -188,6 +188,11 @@ define(function LiveDevelopment(require, exports, module) {
* @type {BaseServer}
*/
var _server;
+
+ /**
+ * Handlers of registered servers
@busykai
busykai Apr 19, 2015 Member

Mark it @private.

@busykai
busykai Apr 19, 2015 Member

"Handlers" => "Handles"

@busykai busykai commented on an outdated diff Apr 19, 2015
src/LiveDevelopment/LiveDevServerManager.js
@@ -88,6 +89,7 @@ define(function (require, exports, module) {
* particular url. Providers that register with a higher priority will
* have the opportunity to provide a given url before those with a
* lower priority. The higher the number, the higher the priority.
+ * @return {{create: function():BaseServer, priority: number}}
@busykai
busykai Apr 19, 2015 Member

Since it's only used as an opaque handle, no need to specify it's internals. should be just {object}.

@busykai busykai commented on an outdated diff Apr 19, 2015
src/LiveDevelopment/LiveDevServerManager.js
@@ -102,6 +104,23 @@ define(function (require, exports, module) {
_serverProviders.push(providerObj);
_serverProviders.sort(_providerSort);
+
+ return providerObj;
+ }
+
+ /**
+ * Remove a server from the list of the registered providers.
+ *
+ * @param {{create: function():BaseServer}, priority: number}} provider
@busykai
busykai Apr 19, 2015 Member

Should be just {object}.

@busykai busykai commented on an outdated diff Apr 19, 2015
src/LiveDevelopment/LiveDevServerManager.js
@@ -102,6 +104,23 @@ define(function (require, exports, module) {
_serverProviders.push(providerObj);
_serverProviders.sort(_providerSort);
+
+ return providerObj;
+ }
+
+ /**
+ * Remove a server from the list of the registered providers.
+ *
+ * @param {{create: function():BaseServer}, priority: number}} provider
+ * The provider to be removed.
@busykai
busykai Apr 19, 2015 Member

Do not start this on a new line, should be on the same line as @param.

@busykai busykai commented on the diff Apr 19, 2015
src/LiveDevelopment/LiveDevelopment.js
@@ -868,6 +873,11 @@ define(function LiveDevelopment(require, exports, module) {
var closeDeferred = (brackets.platform === "mac") ? NativeApp.closeLiveBrowser() : $.Deferred().resolve();
closeDeferred.done(function () {
_setStatus(STATUS_INACTIVE, reason || "explicit_close");
+ // clean-up registered servers
+ _regServers.forEach(function (server) {
+ LiveDevServerManager.removeServer(server);
@busykai
busykai Apr 19, 2015 Member

We don't close servers when the LivePreview session closes. Perhaps, it would be the right moment to close the server. This, though, will require exposing closeServer in at least StaticServer... It seems like the node domain command is there, but it's never used. I'll file a separate issue.

@sebaslv
sebaslv Apr 24, 2015 Contributor

Yes, that would be good since the server keeps running after closing the session. Let me know if you've created the issue, I can take a look and submit a PR for that.

@busykai
Member
busykai commented Apr 19, 2015

@sebaslv, done with the review. Just these minor comments.

@sebaslv
Contributor
sebaslv commented Apr 24, 2015

@kai, I've already included those changes. Thanks!

@busykai
Member
busykai commented May 13, 2015

someone named 'kai' got surprised. :)

@busykai
Member
busykai commented May 14, 2015

LGTM. Merging.

@busykai busykai merged commit 76838a4 into adobe:master May 14, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment