client: handle errors in connectAndInspect #105
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Marking it semver-major because of the changed prototype of the resulting object. I'll open a backport PR for v0.6 when this one is approved. |
As `DataCollector` always returns the data it managed to collect
successfully and passes errors as a separate object, `connectAndInspect`
has put errors as `_errors` property of the result to be consistent.
This was a horrible design decision that was counter-intuitive and would
only lead to the lack of proper error handling in application code (and
has actually led in our integration test).
Worse than that, even this `_errors` property would never have been
populated with errors because of a bug in `connectAndInspect`. A patch
for the bug (just in case we ever want to backport it):
diff --git a/lib/client.js b/lib/client.js
index 83236d9..b0436fd 100644
--- a/lib/client.js
+++ b/lib/client.js
@@ -124,7 +124,7 @@ Client.prototype.connectAndInspect = function(
interfaces.forEach((interfaceName) => {
connection.inspectInterface(interfaceName, (error, appInterface) => {
if (error) {
- appInterface = null;
+ appInterface = error;
}
collector.collect(interfaceName, appInterface);
});
The proper solution would have been to stop inspecting interfaces as
soon as the first error occurs and return it as the first argument of
the callback. As it turned out, it was quite tricky to do with
DataCollector, and some other issues were found while inspecting
`metasync` sources too. Fixing these problems requires
backwards-incompatible changes and will certainly happen in the next
semver-major release of `metasync`, but we need a quick fix here. For
this reason, the function was refactored to use promises.
aqrln
force-pushed
the
connectAndInspect-promise
branch
from
1ad125f
to
35961e5
Compare
|
Rebased to resolve conflicts. |
aqrln
added a commit
that referenced
this pull request
Mar 21, 2017
As `DataCollector` always returns the data it managed to collect
successfully and passes errors as a separate object, `connectAndInspect`
has put errors as `_errors` property of the result to be consistent.
This was a horrible design decision that was counter-intuitive and would
only lead to the lack of proper error handling in application code (and
has actually led in our integration test).
Worse than that, even this `_errors` property would never have been
populated with errors because of a bug in `connectAndInspect`. A patch
for the bug (just in case we ever want to backport it):
diff --git a/lib/client.js b/lib/client.js
index 83236d9..b0436fd 100644
--- a/lib/client.js
+++ b/lib/client.js
@@ -124,7 +124,7 @@ Client.prototype.connectAndInspect = function(
interfaces.forEach((interfaceName) => {
connection.inspectInterface(interfaceName, (error, appInterface) => {
if (error) {
- appInterface = null;
+ appInterface = error;
}
collector.collect(interfaceName, appInterface);
});
The proper solution would have been to stop inspecting interfaces as
soon as the first error occurs and return it as the first argument of
the callback. As it turned out, it was quite tricky to do with
DataCollector, and some other issues were found while inspecting
`metasync` sources too. Fixing these problems requires
backwards-incompatible changes and will certainly happen in the next
semver-major release of `metasync`, but we need a quick fix here. For
this reason, the function was refactored to use promises.
PR-URL: #105
Reviewed-by: Mykola Bilochub <nbelochub@gmail.com>
|
Landed in d8ba3fe. |
|
Well, it can't really be converted to semver-patch since there's no |
|
@belochub though, given it has an underscore in its name, can we say that it's an undocumented implementation detail and the bug was that |
|
@belochub huh, I think we indeed can. This |
|
@aqrln, yeah, you are right. |
|
Backported in #106. |
aqrln
added a commit
that referenced
this pull request
Apr 3, 2017
This is kind of backports PR #105 in a different way, and this way might be even better than that the original commit used. The reason of doing this for v0.5, besides fixing a bug, is the fact that we cannot use metasync in v0.5 since it is not compatible with old Node.js versions while old library versions that used to be are too outdated and even get reported by bitHound as insecure. Backport-of: #105
aqrln
added a commit
that referenced
this pull request
Apr 3, 2017
This is kind of backports PR #105 in a different way, and this way might be even better than that the original commit used. The reason of doing this for v0.5, besides fixing a bug, is the fact that we cannot use metasync in v0.5 since it is not compatible with old Node.js versions while old library versions that used to be are too outdated and even get reported by bitHound as insecure. Backport-of: #105 PR-URL: #120 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>
aqrln
added a commit
that referenced
this pull request
Apr 3, 2017
This is kind of backports PR #105 in a different way, and this way might be even better than that the original commit used. The reason of doing this for v0.5, besides fixing a bug, is the fact that we cannot use metasync in v0.5 since it is not compatible with old Node.js versions while old library versions that used to be are too outdated and even get reported by bitHound as insecure. Backport-of: #105 PR-URL: #120 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>
belochub
pushed a commit
that referenced
this pull request
Jan 22, 2018
As `DataCollector` always returns the data it managed to collect
successfully and passes errors as a separate object, `connectAndInspect`
has put errors as `_errors` property of the result to be consistent.
This was a horrible design decision that was counter-intuitive and would
only lead to the lack of proper error handling in application code (and
has actually led in our integration test).
Worse than that, even this `_errors` property would never have been
populated with errors because of a bug in `connectAndInspect`. A patch
for the bug (just in case we ever want to backport it):
diff --git a/lib/client.js b/lib/client.js
index 83236d9..b0436fd 100644
--- a/lib/client.js
+++ b/lib/client.js
@@ -124,7 +124,7 @@ Client.prototype.connectAndInspect = function(
interfaces.forEach((interfaceName) => {
connection.inspectInterface(interfaceName, (error, appInterface) => {
if (error) {
- appInterface = null;
+ appInterface = error;
}
collector.collect(interfaceName, appInterface);
});
The proper solution would have been to stop inspecting interfaces as
soon as the first error occurs and return it as the first argument of
the callback. As it turned out, it was quite tricky to do with
DataCollector, and some other issues were found while inspecting
`metasync` sources too. Fixing these problems requires
backwards-incompatible changes and will certainly happen in the next
semver-major release of `metasync`, but we need a quick fix here. For
this reason, the function was refactored to use promises.
PR-URL: #105
Reviewed-by: Mykola Bilochub <nbelochub@gmail.com>
belochub
pushed a commit
that referenced
this pull request
Jan 22, 2018
As `DataCollector` always returns the data it managed to collect
successfully and passes errors as a separate object, `connectAndInspect`
has put errors as `_errors` property of the result to be consistent.
This was a horrible design decision that was counter-intuitive and would
only lead to the lack of proper error handling in application code (and
has actually led in our integration test).
Worse than that, even this `_errors` property would never have been
populated with errors because of a bug in `connectAndInspect`. A patch
for the bug (just in case we ever want to backport it):
diff --git a/lib/client.js b/lib/client.js
index 83236d9..b0436fd 100644
--- a/lib/client.js
+++ b/lib/client.js
@@ -124,7 +124,7 @@ Client.prototype.connectAndInspect = function(
interfaces.forEach((interfaceName) => {
connection.inspectInterface(interfaceName, (error, appInterface) => {
if (error) {
- appInterface = null;
+ appInterface = error;
}
collector.collect(interfaceName, appInterface);
});
The proper solution would have been to stop inspecting interfaces as
soon as the first error occurs and return it as the first argument of
the callback. As it turned out, it was quite tricky to do with
DataCollector, and some other issues were found while inspecting
`metasync` sources too. Fixing these problems requires
backwards-incompatible changes and will certainly happen in the next
semver-major release of `metasync`, but we need a quick fix here. For
this reason, the function was refactored to use promises.
PR-URL: #105
Reviewed-by: Mykola Bilochub <nbelochub@gmail.com>
belochub
added a commit
that referenced
this pull request
Jan 22, 2018
This is a new and shiny first major release for `metarhia-jstp`.
Changes include API refactoring and improvements, implementations of
CLI, sessions, and application versions, native addon build optimizations,
lots of bug fixes, test coverage increase, and other, less notable changes.
This release also denotes the bump of the protocol version to v1.0.
The only difference from the previous version of the protocol is that
"old" heartbeat messages (`{}`) are now deprecated and `ping`/`pong`
messages must be used for this purpose instead.
Notable changes:
* **src,build:** improve the native module subsystem
*(Alexey Orlenko)*
[#36](#36)
**\[semver-minor\]**
* **build:** compile in ISO C++11 mode
*(Alexey Orlenko)*
[#37](#37)
**\[semver-minor\]**
* **build:** improve error handling
*(Alexey Orlenko)*
[#40](#40)
**\[semver-minor\]**
* **lib:** refactor record-serialization.js
*(Alexey Orlenko)*
[#41](#41)
**\[semver-minor\]**
* **protocol:** change the format of handshake packets
*(Alexey Orlenko)*
[#54](#54)
**\[semver-major\]**
* **parser:** remove special case for '\0' literal
*(Mykola Bilochub)*
[#68](#68)
**\[semver-major\]**
* **client:** drop redundant callback argument
*(Alexey Orlenko)*
[#104](#104)
**\[semver-major\]**
* **client:** handle errors in connectAndInspect
*(Alexey Orlenko)*
[#105](#105)
**\[semver-major\]**
* **socket,ws:** use socket.destroy() properly
*(Alexey Orlenko)*
[#84](#84)
**\[semver-major\]**
* **cli:** add basic implementation
*(Mykola Bilochub)*
[#107](#107)
**\[semver-minor\]**
* **connection:** fix error handling in optional cbs
*(Alexey Orlenko)*
[#147](#147)
**\[semver-major\]**
* **lib:** change event signature
*(Denys Otrishko)*
[#187](#187)
**\[semver-major\]**
* **lib:** add address method to Server
*(Denys Otrishko)*
[#190](#190)
**\[semver-minor\]**
* **lib:** optimize connection events
*(Denys Otrishko)*
[#222](#222)
**\[semver-major\]**
* **lib:** refactor server and client API
*(Denys Otrishko)*
[#209](#209)
**\[semver-major\]**
* **lib,src:** rename term packet usages to message
*(Denys Otrishko)*
[#270](#270)
**\[semver-major\]**
* **lib:** emit events about connection messages
*(Denys Otrishko)*
[#252](#252)
**\[semver-minor\]**
* **connection:** make callback method private
*(Alexey Orlenko)*
[#306](#306)
**\[semver-major\]**
* **lib:** implement sessions
*(Mykola Bilochub)*
[#289](#289)
**\[semver-major\]**
* **connection:** use ping-pong instead of heartbeat
*(Dmytro Nechai)*
[#303](#303)
**\[semver-major\]**
Merged
belochub
added a commit
that referenced
this pull request
Jan 23, 2018
This is a new and shiny first major release for `metarhia-jstp`.
Changes include API refactoring and improvements, implementations of
CLI, sessions, and application versions, native addon build optimizations,
lots of bug fixes, test coverage increase, and other, less notable changes.
This release also denotes the bump of the protocol version to v1.0.
The only difference from the previous version of the protocol is that
"old" heartbeat messages (`{}`) are now deprecated and `ping`/`pong`
messages must be used for this purpose instead.
Notable changes:
* **src,build:** improve the native module subsystem
*(Alexey Orlenko)*
[#36](#36)
**\[semver-minor\]**
* **build:** compile in ISO C++11 mode
*(Alexey Orlenko)*
[#37](#37)
**\[semver-minor\]**
* **build:** improve error handling
*(Alexey Orlenko)*
[#40](#40)
**\[semver-minor\]**
* **lib:** refactor record-serialization.js
*(Alexey Orlenko)*
[#41](#41)
* **parser:** fix a possible memory leak
*(Alexey Orlenko)*
[#44](#44)
**\[semver-minor\]**
* **protocol:** change the format of handshake packets
*(Alexey Orlenko)*
[#54](#54)
**\[semver-major\]**
* **parser:** make parser single-pass
*(Mykola Bilochub)*
[#61](#61)
* **parser:** remove special case for '\0' literal
*(Mykola Bilochub)*
[#68](#68)
**\[semver-major\]**
* **parser:** fix bug causing node to crash
*(Mykola Bilochub)*
[#75](#75)
* **client:** drop redundant callback argument
*(Alexey Orlenko)*
[#104](#104)
**\[semver-major\]**
* **client:** handle errors in connectAndInspect
*(Alexey Orlenko)*
[#105](#105)
**\[semver-major\]**
* **socket,ws:** use socket.destroy() properly
*(Alexey Orlenko)*
[#84](#84)
**\[semver-major\]**
* **cli:** add basic implementation
*(Mykola Bilochub)*
[#107](#107)
**\[semver-minor\]**
* **connection:** fix error handling in optional cbs
*(Alexey Orlenko)*
[#147](#147)
**\[semver-major\]**
* **test:** add JSON5 specs test suite
*(Alexey Orlenko)*
[#158](#158)
* **lib:** change event signature
*(Denys Otrishko)*
[#187](#187)
**\[semver-major\]**
* **lib:** add address method to Server
*(Denys Otrishko)*
[#190](#190)
**\[semver-minor\]**
* **parser:** implement NaN and Infinity parsing
*(Mykola Bilochub)*
[#201](#201)
* **parser:** improve string parsing performance
*(Mykola Bilochub)*
[#220](#220)
* **lib:** optimize connection events
*(Denys Otrishko)*
[#222](#222)
**\[semver-major\]**
* **lib:** refactor server and client API
*(Denys Otrishko)*
[#209](#209)
**\[semver-major\]**
* **lib,src:** rename term packet usages to message
*(Denys Otrishko)*
[#270](#270)
**\[semver-major\]**
* **lib:** emit events about connection messages
*(Denys Otrishko)*
[#252](#252)
**\[semver-minor\]**
* **lib:** implement API versioning
*(Denys Otrishko)*
[#231](#231)
**\[semver-minor\]**
* **lib:** allow to set event handlers in application
*(Denys Otrishko)*
[#286](#286)
**\[semver-minor\]**
* **lib:** allow to broadcast events from server
*(Denys Otrishko)*
[#287](#287)
**\[semver-minor\]**
* **connection:** make callback method private
*(Alexey Orlenko)*
[#306](#306)
**\[semver-major\]**
* **lib:** implement sessions
*(Mykola Bilochub)*
[#289](#289)
**\[semver-major\]**
* **connection:** use ping-pong instead of heartbeat
*(Dmytro Nechai)*
[#303](#303)
**\[semver-major\]**
belochub
pushed a commit
to metarhia/mdsf
that referenced
this pull request
Jul 19, 2018
As `DataCollector` always returns the data it managed to collect
successfully and passes errors as a separate object, `connectAndInspect`
has put errors as `_errors` property of the result to be consistent.
This was a horrible design decision that was counter-intuitive and would
only lead to the lack of proper error handling in application code (and
has actually led in our integration test).
Worse than that, even this `_errors` property would never have been
populated with errors because of a bug in `connectAndInspect`. A patch
for the bug (just in case we ever want to backport it):
diff --git a/lib/client.js b/lib/client.js
index 83236d9..b0436fd 100644
--- a/lib/client.js
+++ b/lib/client.js
@@ -124,7 +124,7 @@ Client.prototype.connectAndInspect = function(
interfaces.forEach((interfaceName) => {
connection.inspectInterface(interfaceName, (error, appInterface) => {
if (error) {
- appInterface = null;
+ appInterface = error;
}
collector.collect(interfaceName, appInterface);
});
The proper solution would have been to stop inspecting interfaces as
soon as the first error occurs and return it as the first argument of
the callback. As it turned out, it was quite tricky to do with
DataCollector, and some other issues were found while inspecting
`metasync` sources too. Fixing these problems requires
backwards-incompatible changes and will certainly happen in the next
semver-major release of `metasync`, but we need a quick fix here. For
this reason, the function was refactored to use promises.
PR-URL: metarhia/jstp#105
Reviewed-by: Mykola Bilochub <nbelochub@gmail.com>
belochub
pushed a commit
to metarhia/mdsf
that referenced
this pull request
Jul 19, 2018
As `DataCollector` always returns the data it managed to collect
successfully and passes errors as a separate object, `connectAndInspect`
has put errors as `_errors` property of the result to be consistent.
This was a horrible design decision that was counter-intuitive and would
only lead to the lack of proper error handling in application code (and
has actually led in our integration test).
Worse than that, even this `_errors` property would never have been
populated with errors because of a bug in `connectAndInspect`. A patch
for the bug (just in case we ever want to backport it):
diff --git a/lib/client.js b/lib/client.js
index 83236d9..b0436fd 100644
--- a/lib/client.js
+++ b/lib/client.js
@@ -124,7 +124,7 @@ Client.prototype.connectAndInspect = function(
interfaces.forEach((interfaceName) => {
connection.inspectInterface(interfaceName, (error, appInterface) => {
if (error) {
- appInterface = null;
+ appInterface = error;
}
collector.collect(interfaceName, appInterface);
});
The proper solution would have been to stop inspecting interfaces as
soon as the first error occurs and return it as the first argument of
the callback. As it turned out, it was quite tricky to do with
DataCollector, and some other issues were found while inspecting
`metasync` sources too. Fixing these problems requires
backwards-incompatible changes and will certainly happen in the next
semver-major release of `metasync`, but we need a quick fix here. For
this reason, the function was refactored to use promises.
PR-URL: metarhia/jstp#105
Reviewed-by: Mykola Bilochub <nbelochub@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As
DataCollectoralways returns the data it managed to collectsuccessfully and passes errors as a separate object,
connectAndInspecthas put errors as
_errorsproperty of the result to be consistent.This was a horrible design decision that was counter-intuitive and would
only lead to the lack of proper error handling in application code (and
has actually led in our integration test).
Worse than that, even this
_errorsproperty would never have beenpopulated with errors because of a bug in
connectAndInspect. A patchfor the bug (just in case we ever want to backport it):
The proper solution would have been to stop inspecting interfaces as
soon as the first error occurs and return it as the first argument of
the callback. As it turned out, it was quite tricky to do with
DataCollector, and some other issues were found while inspecting
metasyncsources too. Fixing these problems requiresbackwards-incompatible changes and will certainly happen in the next
semver-major release of
metasync, but we need a quick fix here. Forthis reason, the function was refactored to use promises.