Change logic around closing connections and writers API-1283 #1417

srknzl · 2022-11-17T14:06:44Z

The issue was writing some data to the connection after it being closed. I changed and refactored some logic around PipelinedWriter and Connection to avoid that.

Moved socket closing responsibility to Writers
Fixed a test in PipelinedWriterTest
Ongoing and future socket.write calls will end with an error after the writer is closed. This is because close() destroys the socket and any ongoing and future socket.write calls will end with an error as described in node.js documentation. So we will be able to reject all deferred promises in the write queue upon close due to connection.close() or socket write error close().
We were not closing the socket on write error before, now we do it. We were sending end packet and don't write anymore. This is the same as java. See

srknzl · 2022-11-17T14:08:00Z

test/unit/connection/PipelinedWriterTest.js

        const err = new Error();
        setUpWriteFailure(err);

        const resolver = deferredPromise();
        writer.write(createMessageFromString('test'), resolver);
-        resolver.promise.catch((err) => {
-            expect(err).to.be.equal(err);


This was strange, basically making 1 === 1 assertion. It does not work if I rename one of the variables because we were wraping (and still doing) the err in IOError. (in a defected way we pass error as message, fixed that as well.)

codecov · 2022-11-18T07:05:50Z

Codecov Report

Merging #1417 (96d534f) into master (2ac53b4) will decrease coverage by 0.07%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master    #1417      +/-   ##
==========================================
- Coverage   93.27%   93.20%   -0.08%     
==========================================
  Files         464      466       +2     
  Lines       16386    16623     +237     
  Branches     1333     1351      +18     
==========================================
+ Hits        15284    15493     +209     
- Misses        801      827      +26     
- Partials      301      303       +2

Impacted Files	Coverage Δ
src/network/Connection.ts	`94.82% <84.61%> (+0.60%)`	⬆️
src/network/ConnectionManager.ts	`79.81% <0.00%> (-5.17%)`	⬇️
src/core/UUID.ts	`85.00% <0.00%> (-5.00%)`	⬇️
src/codec/SqlExecuteCodec.ts	`95.74% <0.00%> (-4.26%)`	⬇️
src/util/Util.ts	`86.30% <0.00%> (-1.60%)`	⬇️
src/protocol/ErrorFactory.ts	`64.66% <0.00%> (-0.76%)`	⬇️
src/core/HazelcastError.ts	`77.61% <0.00%> (-0.42%)`	⬇️
src/serialization/ObjectData.ts	`88.93% <0.00%> (-0.40%)`	⬇️
src/config/ConfigBuilder.ts	`90.29% <0.00%> (-0.39%)`	⬇️
src/core/index.ts	`100.00% <0.00%> (ø)`
... and 121 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

srknzl · 2022-11-18T11:37:56Z

src/network/Connection.ts

        }
        for (const item of this.queue) {
-            item.resolver.reject(this.error);
+            item.resolver.reject(error);


This line is added but not covered, and this is normal. Actually there was no test before my change as well. I did not change the logic of here. Just added this.close() to the end of this function. I don't want to add a test for this because it is not trivial and requires some magic with sinon.

src/network/Connection.ts

srknzl · 2022-12-02T08:37:19Z

Note regarding coverage: The coverage drop is shown as 0.16% but I see that the drop is due to SSL tests because the drop is in connectTLSSocket. I think it should be related to the ssl tests being disabled. (Due to certificate issue)

Also note that codecov does not show a line that is added but not covered in this PR. (except a single line, I explained it above)

src/network/Connection.ts

harunalpak

I reviewed all codes and tests, it seems good. I could not find any problem. I have only one question that I am not sure. After response I can approve.

…azelcast#1417)" This reverts commit 2d0b397.

…1417)" (#1430) This reverts commit 2d0b397.

…st#1417) The issue was writing some data to the connection after it being closed. I changed and refactored some logic around PipelinedWriter and Connection to avoid that. - Moved socket closing responsibility to Writers - Fixed a test in PipelinedWriterTest - Ongoing and future socket.write calls will end with an error after the writer is closed. This is because close() destroys the socket and any ongoing and future socket.write calls will end with an error as described in node.js [documentation](https://nodejs.org/api/stream.html#writabledestroyerror). So we will be able to reject all deferred promises in the write queue upon close due to connection.close() or socket write error close(). - We were not closing the socket on write error before, now we do it. We were sending end packet and don't write anymore. This is the same as java. [See](https://hazelcast.slack.com/archives/C01JU7ZJYGP/p1668695419424179) fixes hazelcast#1256

…azelcast#1417)" (hazelcast#1430) This reverts commit 2d0b397.

…st#1417) The issue was writing some data to the connection after it being closed. I changed and refactored some logic around PipelinedWriter and Connection to avoid that. - Moved socket closing responsibility to Writers - Fixed a test in PipelinedWriterTest - Ongoing and future socket.write calls will end with an error after the writer is closed. This is because close() destroys the socket and any ongoing and future socket.write calls will end with an error as described in node.js [documentation](https://nodejs.org/api/stream.html#writabledestroyerror). So we will be able to reject all deferred promises in the write queue upon close due to connection.close() or socket write error close(). - We were not closing the socket on write error before, now we do it. We were sending end packet and don't write anymore. This is the same as java. [See](https://hazelcast.slack.com/archives/C01JU7ZJYGP/p1668695419424179) fixes hazelcast#1256

…azelcast#1417)" (hazelcast#1430) This reverts commit 2d0b397.

…st#1417) The issue was writing some data to the connection after it being closed. I changed and refactored some logic around PipelinedWriter and Connection to avoid that. - Moved socket closing responsibility to Writers - Fixed a test in PipelinedWriterTest - Ongoing and future socket.write calls will end with an error after the writer is closed. This is because close() destroys the socket and any ongoing and future socket.write calls will end with an error as described in node.js [documentation](https://nodejs.org/api/stream.html#writabledestroyerror). So we will be able to reject all deferred promises in the write queue upon close due to connection.close() or socket write error close(). - We were not closing the socket on write error before, now we do it. We were sending end packet and don't write anymore. This is the same as java. [See](https://hazelcast.slack.com/archives/C01JU7ZJYGP/p1668695419424179) fixes hazelcast#1256

…azelcast#1417)" (hazelcast#1430) This reverts commit 2d0b397.

…st#1417) The issue was writing some data to the connection after it being closed. I changed and refactored some logic around PipelinedWriter and Connection to avoid that. - Moved socket closing responsibility to Writers - Fixed a test in PipelinedWriterTest - Ongoing and future socket.write calls will end with an error after the writer is closed. This is because close() destroys the socket and any ongoing and future socket.write calls will end with an error as described in node.js [documentation](https://nodejs.org/api/stream.html#writabledestroyerror). So we will be able to reject all deferred promises in the write queue upon close due to connection.close() or socket write error close(). - We were not closing the socket on write error before, now we do it. We were sending end packet and don't write anymore. This is the same as java. [See](https://hazelcast.slack.com/archives/C01JU7ZJYGP/p1668695419424179) fixes hazelcast#1256

…azelcast#1417)" (hazelcast#1430) This reverts commit 2d0b397.

…st#1417) The issue was writing some data to the connection after it being closed. I changed and refactored some logic around PipelinedWriter and Connection to avoid that. - Moved socket closing responsibility to Writers - Fixed a test in PipelinedWriterTest - Ongoing and future socket.write calls will end with an error after the writer is closed. This is because close() destroys the socket and any ongoing and future socket.write calls will end with an error as described in node.js [documentation](https://nodejs.org/api/stream.html#writabledestroyerror). So we will be able to reject all deferred promises in the write queue upon close due to connection.close() or socket write error close(). - We were not closing the socket on write error before, now we do it. We were sending end packet and don't write anymore. This is the same as java. [See](https://hazelcast.slack.com/archives/C01JU7ZJYGP/p1668695419424179) fixes hazelcast#1256

…azelcast#1417)" (hazelcast#1430) This reverts commit 2d0b397.

srknzl added 3 commits November 17, 2022 16:31

Fix pipelined writer

347d800

More changes

e097433

More changes

6ba6653

srknzl added Type: Test-Failure Priority: High Estimation: S Source: Internal labels Nov 17, 2022

srknzl requested review from mdumandag and harunalpak November 17, 2022 14:06

srknzl added this to the 5.2.0 milestone Nov 17, 2022

srknzl commented Nov 17, 2022

View reviewed changes

srknzl added 2 commits November 17, 2022 17:14

Destroy socket in DirectWriter upon close

780702b

Fix cause usage

5e08c6f

Add some tests

6cf5f31

srknzl commented Nov 18, 2022

View reviewed changes

Add a test to DirectWriterTest

12e470b

srknzl changed the title ~~Fix pipelined writer API-1283~~ Change logic around closing connections and writers API-1283 Nov 18, 2022

mdumandag reviewed Dec 1, 2022

View reviewed changes

src/network/Connection.ts Outdated Show resolved Hide resolved

Change Writer.close(Error|null) to Write.close(Error)

a32fa9b

mdumandag reviewed Dec 2, 2022

View reviewed changes

src/network/Connection.ts Outdated Show resolved Hide resolved

Remove isClosed method and rename closeCause to error

43b135c

mdumandag previously approved these changes Dec 2, 2022

View reviewed changes

harunalpak reviewed Dec 5, 2022

View reviewed changes

src/network/Connection.ts Outdated Show resolved Hide resolved

harunalpak reviewed Dec 5, 2022

View reviewed changes

Add cause to DirectWriter.close

96d534f

srknzl dismissed mdumandag’s stale review via 96d534f December 5, 2022 12:23

harunalpak approved these changes Dec 5, 2022

View reviewed changes

mdumandag approved these changes Dec 5, 2022

View reviewed changes

srknzl merged commit 2d0b397 into hazelcast:master Dec 5, 2022

srknzl added a commit to srknzl/hazelcast-nodejs-client that referenced this pull request Dec 5, 2022

Revert "Change logic around closing connections and writers API-1283 (h…

6c49813

…azelcast#1417)" This reverts commit 2d0b397.

srknzl added a commit that referenced this pull request Dec 5, 2022

Revert "Change logic around closing connections and writers API-1283 (#…

67ea00f

…1417)" (#1430) This reverts commit 2d0b397.

harunalpak pushed a commit to harunalpak/hazelcast-nodejs-client that referenced this pull request Dec 7, 2022

Revert "Change logic around closing connections and writers API-1283 (h…

a760d53

…azelcast#1417)" (hazelcast#1430) This reverts commit 2d0b397.

harunalpak pushed a commit to harunalpak/hazelcast-nodejs-client that referenced this pull request Dec 8, 2022

Revert "Change logic around closing connections and writers API-1283 (h…

53000b5

…azelcast#1417)" (hazelcast#1430) This reverts commit 2d0b397.

harunalpak pushed a commit to harunalpak/hazelcast-nodejs-client that referenced this pull request Dec 8, 2022

Revert "Change logic around closing connections and writers API-1283 (h…

641f053

…azelcast#1417)" (hazelcast#1430) This reverts commit 2d0b397.

harunalpak pushed a commit to harunalpak/hazelcast-nodejs-client that referenced this pull request Dec 8, 2022

Revert "Change logic around closing connections and writers API-1283 (h…

f75acdc

…azelcast#1417)" (hazelcast#1430) This reverts commit 2d0b397.

harunalpak pushed a commit to harunalpak/hazelcast-nodejs-client that referenced this pull request Dec 8, 2022

Revert "Change logic around closing connections and writers API-1283 (h…

6bbe14a

…azelcast#1417)" (hazelcast#1430) This reverts commit 2d0b397.

harunalpak pushed a commit to harunalpak/hazelcast-nodejs-client that referenced this pull request Dec 8, 2022

Revert "Change logic around closing connections and writers API-1283 (h…

81135a1

…azelcast#1417)" (hazelcast#1430) This reverts commit 2d0b397.

harunalpak pushed a commit to harunalpak/hazelcast-nodejs-client that referenced this pull request Dec 8, 2022

Revert "Change logic around closing connections and writers API-1283 (h…

4ca0cb3

…azelcast#1417)" (hazelcast#1430) This reverts commit 2d0b397.

srknzl mentioned this pull request Dec 13, 2022

Change logic around closing connections and writers API-1283 (redux) #1442

Merged

srknzl added a commit to fatihozer0/hazelcast-nodejs-client that referenced this pull request Mar 24, 2023

Revert "Change logic around closing connections and writers API-1283 (h…

e7d8a8e

…azelcast#1417)" (hazelcast#1430) This reverts commit 2d0b397.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change logic around closing connections and writers API-1283 #1417

Change logic around closing connections and writers API-1283 #1417

srknzl commented Nov 17, 2022 •

edited

Loading

srknzl Nov 17, 2022 •

edited

Loading

codecov bot commented Nov 18, 2022 •

edited

Loading

srknzl Nov 18, 2022

srknzl commented Dec 2, 2022 •

edited

Loading

harunalpak left a comment

Change logic around closing connections and writers API-1283 #1417

Change logic around closing connections and writers API-1283 #1417

Conversation

srknzl commented Nov 17, 2022 • edited Loading

srknzl Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 18, 2022 • edited Loading

Codecov Report

srknzl Nov 18, 2022

Choose a reason for hiding this comment

srknzl commented Dec 2, 2022 • edited Loading

harunalpak left a comment

Choose a reason for hiding this comment

srknzl commented Nov 17, 2022 •

edited

Loading

srknzl Nov 17, 2022 •

edited

Loading

codecov bot commented Nov 18, 2022 •

edited

Loading

srknzl commented Dec 2, 2022 •

edited

Loading