Closed Bug 1293982 Opened 8 years ago Closed 8 years ago

TcpTransport.close() should force a shutdown of the socket

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox50 fixed, firefox51 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(
URL
)

Details

Attachments

(1 file)

Bug 1293982 - TcpTransport.close() has to force a shutdown of the socket.
58 bytes, text/x-review-board-request
ato
: review+
Details 
Right now we are calling `self.client.close()` in Marionette which will cause the socket to be closed. BUT, the socket will still remain for a moment on the server side. This can cause race conditions when eg after an application restart the client wants to connect to the application again.

Please see the Python socket documentation:
https://docs.python.org/2/library/socket.html#socket.socket.close

> close() releases the resource associated with a connection but does not
> necessarily close the connection immediately. If you want to close the
> connection in a timely fashion, call shutdown() before close(). 

I would propose that we make use of shutdown():
https://docs.python.org/2/library/socket.html#socket.socket.shutdown

Given that we don't want to submit or receive anything else we should call this method with `SHUT_RDWR`.

This bug is a potential candidate for bug 1293404 and bug 1257508.

Andreas, do you agree to my suggestion? Or is there something what I miss here?
Flags: needinfo?(ato)
Btw the following page gives a kinda deep introduction into why shutdown() should always be used when working with sockets in Python:

https://engineering.imvu.com/2014/03/06/charming-python-how-to-actually-close-a-socket-by-calling-shutdown-before-calling-close/
URL: https://engineering.imvu.com/2014/03/...
Comment on attachment 8781099 [details]
Bug 1293982 - TcpTransport.close() has to force a shutdown of the socket.

https://reviewboard.mozilla.org/r/71602/#review69078

::: testing/marionette/client/marionette_driver/transport.py:212
(Diff revision 1)
>  
>          Returns a tuple of the protocol level and the application type.
>          """
>          try:
> +            self.sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
> +            self.sock.settimeout(self.socket_timeout)

This partly reverts changes as made by the Marionette protocol level 3 patch:

https://hg.mozilla.org/mozilla-central/rev/4cf388b1cf3a

Andreas let me know if that is fine. I wasn't able to find an indication why this has been moved around.
Flags: needinfo?(ato)
A really nasty bug which I found now by accident is in that line:

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette_driver/transport.py#297

This could clearly be a reason why we sometimes fail to properly close the channel, given that there might still be unused and already freed client references cached.
Comment on attachment 8781099 [details]
Bug 1293982 - TcpTransport.close() has to force a shutdown of the socket.

https://reviewboard.mozilla.org/r/71602/#review69078

> This partly reverts changes as made by the Marionette protocol level 3 patch:
> 
> https://hg.mozilla.org/mozilla-central/rev/4cf388b1cf3a
> 
> Andreas let me know if that is fine. I wasn't able to find an indication why this has been moved around.

I moved it to the ctor because they are just initialising the socket, and not doing any actual attempt at establishing a connection.  This was under the pretense that the class gets constructed once for every socket connection, which is obviously not the case.

I believe your change is more correct and in line with the realities of how the class is used.
Comment on attachment 8781099 [details]
Bug 1293982 - TcpTransport.close() has to force a shutdown of the socket.

https://reviewboard.mozilla.org/r/71602/#review69078

> I moved it to the ctor because they are just initialising the socket, and not doing any actual attempt at establishing a connection.  This was under the pretense that the class gets constructed once for every socket connection, which is obviously not the case.
> 
> I believe your change is more correct and in line with the realities of how the class is used.

By the way: Unfortuantely I cannot resolve issues opened by others.
Comment on attachment 8781099 [details]
Bug 1293982 - TcpTransport.close() has to force a shutdown of the socket.

https://reviewboard.mozilla.org/r/71602/#review70662
Attachment #8781099 - Flags: review?(ato) → review+
This is good to know after four+ yearrs in production…
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/60f05a3d215f
TcpTransport.close() has to force a shutdown of the socket. r=ato
https://hg.mozilla.org/mozilla-central/rev/60f05a3d215f
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
I haven't seen issues with this patch landed so far. Given that we also have lots of crashes on aurora I would suggest to also get it backported to that branch. I don't think we need it for beta.
Assignee: nobody → hskupin
status-firefox50: --- → affected
Whiteboard: [checkin-needed-aurora]
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: