Waiting the remaining tcp data are sent #4501

lygstate · 2021-01-18T11:47:52Z

So that the tcp socket are closed by debugger client first and
the debugger client won't receive socket are closed by remote error

The error message:
Test command:
cmd \
	/S \
	/C \
	C:\hostedtoolcache\windows\Python\3.7.9\x64\python.exe \
	D:\a\jerryscript\jerryscript\tools\runners/run-debugger-test.py \
	D:\a\jerryscript\jerryscript\build\tests\jerry_debugger_tests\local\bin\jerry.exe \
	websocket \
	D:\a\jerryscript\jerryscript\jerry-debugger/jerry_client.py \
	tests\debugger\do_abort
run debug server: D:\a\jerryscript\jerryscript\build\tests\jerry_debugger_tests\local\bin\jerry.exe tests\debugger\do_abort.js --start-debug-server --debug-channel websocket
run debug client: cmd /S /C C:\hostedtoolcache\windows\Python\3.7.9\x64\python.exe D:\a\jerryscript\jerryscript\jerry-debugger/jerry_client.py --channel websocket --non-interactive
input debug cmd: tests\debugger\do_abort.cmd
['git', '--no-pager', 'diff', '--ignore-space-at-eol', '--no-index', 'tests\\debugger\\do_abort.expected', 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\do_abort5k9rpkyiout']
FAIL: tests\debugger\do_abort
warning: LF will be replaced by CRLF in C:\Users\RUNNER~1\AppData\Local\Temp\do_abort5k9rpkyiout.
The file will have its original line endings in your working directory
diff --git "a/tests\\debugger\\do_abort.expected" "b/C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\do_abort5k9rpkyiout"
index 7656e41..8d31910 100644
--- "a/tests\\debugger\\do_abort.expected"
+++ "b/C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\do_abort5k9rpkyiout"
@@ -10,3 +10,5 @@ Stopped at tests/debugger/do_abort.js:20 (in g() at line:19, col:1)
 Stopped at tests/debugger/do_abort.js:16 (in f() at line:15, col:1)
 (jerry-debugger) abort new Error('Fatal error :)')
 err: Error: Fatal error :)
+Failed to connect to the JerryScript debugger.
+Error: [WinError 10054] An existing connection was forcibly closed by the remote host

This is the random failure I am talking about:
https://github.com/jerryscript-project/jerryscript/runs/1721915835

JerryScript-DCO-1.0-Signed-off-by: Yonggang Luo [email protected]

jerry-ext/debugger/debugger-tcp.c

zherczeg · 2021-01-23T07:26:22Z

jerry-ext/debugger/debugger-tcp.c

@@ -144,6 +144,26 @@ jerryx_debugger_tcp_close (jerry_debugger_transport_header_t *header_p) /**< tcp

  jerryx_debugger_transport_tcp_t *tcp_p = (jerryx_debugger_transport_tcp_t *) header_p;

+  /* Waiting the debug client close the tcp connection first */
+  for (;;)


use while(true)

The problem is this loop might never ends.

Yeap, any suggestion? post here looking for suggestion

use while(true)

The problem is this loop might never ends.

maybe we doesn't need consider that, as when debugger stopping, means the debug client always closing the socket?

zherczeg

What about simply ignoring error 10054 on windows?

zherczeg · 2021-02-04T13:02:03Z

jerry-ext/debugger/debugger-tcp.c

@@ -144,6 +144,26 @@ jerryx_debugger_tcp_close (jerry_debugger_transport_header_t *header_p) /**< tcp

  jerryx_debugger_transport_tcp_t *tcp_p = (jerryx_debugger_transport_tcp_t *) header_p;

+  /* Waiting the debug client close the tcp connection first */


Comment: Waiting for the debug client close the tcp connection first.

zherczeg · 2021-02-04T13:04:26Z

jerry-ext/debugger/debugger-tcp.c

+    {
+      /**
+       * If result >= 0, this means that there is either data available on the socket, or the socket
+       * has been closed. So waiting the socket closed by remote client by result == 0


This part breaks the loop. Where is the 'waiting' part?

== 0 means remote closed the tcp socket, > 0 means still have data, < 0 means have error, JERRYX_EWOULDBLOCK error should be ignored(means have no data but the tcp socket still there)

rerobika · 2021-02-04T13:35:47Z

@zherczeg Just to clarify that's not only windows related issue. https://github.com/jerryscript-project/jerryscript/runs/1721915835#step:5:708

So that the tcp socket are closed by debugger client first and the debugger client won't receive socket are closed by remote error JerryScript-DCO-1.0-Signed-off-by: Yonggang Luo [email protected]

zherczeg · 2021-02-05T06:12:39Z

I am still curious: [WinError 10054] An existing connection was forcibly closed by the remote host - this is a hint, not an error, why can't we simply ignore it?

lygstate · 2021-02-05T06:15:49Z

I am still curious: [WinError 10054] An existing connection was forcibly closed by the remote host - this is a hint, not an error, why can't we simply ignore it?

Are you talking about ignore the error in python side?
There is a testcase that restart the jerry engine in debugger, so ignore this error is not acceptable, as you don't know it's a abnormal connection disconnect or an disconnection that caused by the user(the restart debug instruction).

zherczeg · 2021-02-05T06:25:00Z

In the restart debug case you are expecting a restart, it doesn't matter if it fails why it fails. A successful close does not guarantee that the restart will successful. So I still don't get why we cannot ignore this.

lygstate · 2021-02-05T06:45:06Z

In the restart debug case you are expecting a restart, it doesn't matter if it fails why it fails. A successful close does not guarantee that the restart will successful. So I still don't get why we cannot ignore this.

As I said, we can not disinguish normal disconnection(restart instruction) or it's the jerry executable crash(cause connection reset).
So it's better to waiting normal close when restarting, and when crash, [WinError 10054] happens

zherczeg reviewed Jan 18, 2021

View reviewed changes

jerry-ext/debugger/debugger-tcp.c Outdated Show resolved Hide resolved

akosthekiss reviewed Jan 18, 2021

View reviewed changes

jerry-ext/debugger/debugger-tcp.c Show resolved Hide resolved

lygstate closed this Jan 19, 2021

lygstate force-pushed the socket-closed-by-remote branch from ecbbe96 to d97540e Compare January 19, 2021 00:15

lygstate reopened this Jan 23, 2021

lygstate force-pushed the socket-closed-by-remote branch 3 times, most recently from 496b146 to a792221 Compare January 23, 2021 04:19

lygstate requested review from zherczeg and akosthekiss January 23, 2021 04:20

lygstate force-pushed the socket-closed-by-remote branch from a792221 to bb174bc Compare January 23, 2021 04:25

zherczeg requested changes Jan 23, 2021

View reviewed changes

lygstate requested a review from zherczeg January 23, 2021 12:30

lygstate force-pushed the socket-closed-by-remote branch from bb174bc to 1078318 Compare January 23, 2021 14:13

zherczeg requested changes Feb 4, 2021

View reviewed changes

lygstate force-pushed the socket-closed-by-remote branch 2 times, most recently from 60eed79 to 18c2040 Compare February 5, 2021 05:20

Waiting the tcp socket closed by remote client

a2d7a70

So that the tcp socket are closed by debugger client first and the debugger client won't receive socket are closed by remote error JerryScript-DCO-1.0-Signed-off-by: Yonggang Luo [email protected]

lygstate force-pushed the socket-closed-by-remote branch from 18c2040 to a2d7a70 Compare February 5, 2021 05:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Waiting the remaining tcp data are sent #4501

Waiting the remaining tcp data are sent #4501

lygstate commented Jan 18, 2021 •

edited

Loading

zherczeg Jan 23, 2021

lygstate Jan 23, 2021

lygstate Jan 23, 2021

zherczeg left a comment

zherczeg Feb 4, 2021

zherczeg Feb 4, 2021

lygstate Feb 4, 2021 •

edited

Loading

rerobika commented Feb 4, 2021

zherczeg commented Feb 5, 2021

lygstate commented Feb 5, 2021 •

edited

Loading

zherczeg commented Feb 5, 2021

lygstate commented Feb 5, 2021

		@@ -144,6 +144,26 @@ jerryx_debugger_tcp_close (jerry_debugger_transport_header_t header_p) /*< tcp

		jerryx_debugger_transport_tcp_t tcp_p = (jerryx_debugger_transport_tcp_t ) header_p;

		/* Waiting the debug client close the tcp connection first */

Waiting the remaining tcp data are sent #4501

Are you sure you want to change the base?

Waiting the remaining tcp data are sent #4501

Conversation

lygstate commented Jan 18, 2021 • edited Loading

zherczeg Jan 23, 2021

Choose a reason for hiding this comment

lygstate Jan 23, 2021

Choose a reason for hiding this comment

lygstate Jan 23, 2021

Choose a reason for hiding this comment

zherczeg left a comment

Choose a reason for hiding this comment

zherczeg Feb 4, 2021

Choose a reason for hiding this comment

zherczeg Feb 4, 2021

Choose a reason for hiding this comment

lygstate Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

rerobika commented Feb 4, 2021

zherczeg commented Feb 5, 2021

lygstate commented Feb 5, 2021 • edited Loading

zherczeg commented Feb 5, 2021

lygstate commented Feb 5, 2021

lygstate commented Jan 18, 2021 •

edited

Loading

lygstate Feb 4, 2021 •

edited

Loading

lygstate commented Feb 5, 2021 •

edited

Loading