Closed Bug 573227 Opened 12 years ago Closed 12 years ago

.wasClean is randomly false on linux even when it is expected to be true.

Categories

(Core :: DOM: Core & HTML, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: smaug, Assigned: wfernandom2004)

References

Details

(Whiteboard: [fixed by bug 572975])

Attachments

(4 obsolete files)

See
https://bugzilla.mozilla.org/show_bug.cgi?id=562681#c28
https://bugzilla.mozilla.org/show_bug.cgi?id=562681#c33

I don't know what the problem could be. Is it something in pywebsocket?
Is it a bug in necko? Is it a bug in websocket implementation? Or is the
problem in the tests?
blocking2.0: --- → ?
In WebSocket I'm sure that is isn't. In Necko I think it is improbable. Pywebsocket has an issue that doesn't allow to close a connection without sending the close frame, so I think that is the problem. I will take a look at these test fails today.
I enabled tests, but disabled all the wasClean checks. Hopefully tree stays
greener now.
I've just tested the sending of the close frame from pywebsocket. I found out that pywebsocket always sends that frame (even if sys.exit() is called). Because of this the test 15 is impossible to be done, because it expects that frame to not be sent. So I will remove the test 15. Now I will see the tests 9, 13 and 14.
Would it be difficult to add something to pywebsocket so that we could prevent sending the closing handshake in the tests?
No, it wouldn't. We could add a "abort" parameter to close_connection(), or add a new method.
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → wfernandom2004
Status: NEW → ASSIGNED
Attachment #452478 - Flags: review?(Olli.Pettay)
I've reordered these tests:

10 -> 6
6 -> 7
7 -> 6
8 -> 7
9 -> 8
Unfortunately the patch doesn't apply cleanly to trunk.
Could you perhaps update it?
Comment on attachment 452478 [details] [diff] [review]
v1


>-#define TIMEOUT_TRY_CONNECT_AGAIN         1000
>+#define TIMEOUT_TRY_CONNECT_AGAIN         2000
Why you need to change this too?

>-def close_connection(request):
>+def close_connection(request, abort=False):
>     """Close connection.
> 
>     Args:
>         request: mod_python request.
>     """
>     if request.server_terminated:
>         return
>     # 5.3 the server may decide to terminate the WebSocket connection by
>     # running through the following steps:
>     # 1. send a 0xFF byte and a 0x00 byte to the client to indicate the start
>     # of the closing handshake.
>-    _write(request, '\xff\x00')
>+    if not abort:
>+        _write(request, '\xff\x00')
>+        # timeout of 5 seconds to get the client's close frame ack
>+        initial_time = time()
>+        end_time = initial_time + 20
So 5 or 20 seconds? Comment says 5, but code 20.


>+        got_exception = False
>+        while time() < end_time:
>+            try:
>+                receive_message(request)
So does this call block? What if client doesn't send anything?
Or does it not block, which means there is a tight loop up to 20 seconds.

>+            except msgutil.ConnectionTerminatedException, e:
>+                got_exception = True
>+                time.sleep(1)
Why you need this sleep?
Note, when you update the patch, make sure you un-comment-out the tests which
check e.wasClean.
> So does this call block? What if client doesn't send anything?
> Or does it not block, which means there is a tight loop up to 20 seconds.
> Why you need this sleep?
Yes, receive_message() blocks. However if the connection is terminated that call will raise ConnectionTerminatedException without blocking. Because of this there is the sleep of 1 second, in order to prevent a tight loop.
Correction, the reordering is:

10 -> 6
6 -> 7
7 -> 8
8 -> 9
9 -> 10
Attached patch v2 (obsolete) — Splinter Review
Attachment #452478 - Attachment is obsolete: true
Attachment #452502 - Flags: review?(Olli.Pettay)
Attachment #452478 - Flags: review?(Olli.Pettay)
Attachment #452502 - Attachment is obsolete: true
Attachment #452502 - Flags: review?(Olli.Pettay)
Attached patch v2 (obsolete) — Splinter Review
Attachment #452508 - Flags: review?(Olli.Pettay)
Blocks: 572975
Blocks: 573298
I pushed this to tryserver
30345 INFO TEST-PASS | /tests/content/base/test/test_ws_basic_tests.html | Didn't get the huge message back!
Blocks: 573465
Attached patch v3 [superseded by bug 572975] (obsolete) — Splinter Review
I've moved some tests to the end, and some others to the beginning, in order to prevent timeout failures. Olli, can you push this patch in the TryServer, together with the patch I'm going to upload in bug 572975, please?
Attachment #452508 - Attachment is obsolete: true
Attachment #452777 - Flags: review?(Olli.Pettay)
Attachment #452508 - Flags: review?(Olli.Pettay)
Just letting you kbow, the reordering, relative to the path v2 is:
6 -> 2
18 -> 6
17 -> 7
2 -> 17
7 -> 18
So should I push v3 to tryserver, or wait for the new patch in bug 572975?
I've just uploaded the patch in bug 572975. That patch depends on this one to be applied without conflicts.
ok, I'll upload those to tryserver in a few minutes
Comment on attachment 452777 [details] [diff] [review]
v3
[superseded by bug 572975]


>-def close_connection(request):
>+def close_connection(request, abort=False):
>     """Close connection.
> 
>     Args:
>         request: mod_python request.
>     """
>     if request.server_terminated:
>         return
>     # 5.3 the server may decide to terminate the WebSocket connection by
>     # running through the following steps:
>     # 1. send a 0xFF byte and a 0x00 byte to the client to indicate the start
>     # of the closing handshake.
>-    _write(request, '\xff\x00')
>+    got_exception = False
>+    if not abort:
>+        _write(request, '\xff\x00')
>+        # timeout of 20 seconds to get the client's close frame ack
>+        initial_time = time()
>+        end_time = initial_time + 20
>+        while time() < end_time:
>+            try:
>+                receive_message(request)
>+            except ConnectionTerminatedException, e:
>+                got_exception = True
>+                sleep(1)
So if we get the ConnectionTerminatedException, why do we need to still loop?
When it gets the ConnectionTerminatedException it means that the server has received the client's close frame. However it doesn't mean that the close frame that the server hast sent to the client has been received.

Actually, I thought that _write(request, '\xff\x00') would be enough. But somehow without the delay sometimes the client doesn't receive the close frame.
Interesting. Would some kind of flush() call somewhere help with problem that
client doesn't get the frame before the connection closes?
Comment on attachment 452777 [details] [diff] [review]
v3
[superseded by bug 572975]

This landed already in bug 572975
Attachment #452777 - Flags: review?(Olli.Pettay)
Marking this fixed. I haven't seen any failures for awhile.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
blocking2.0: ? → final+
Flags: in-testsuite-
Whiteboard: [fixed by bug 572975]
Attachment #452777 - Attachment description: v3 → v3 [superseded by bug 572975]
Attachment #452777 - Attachment is obsolete: true
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.