Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a user specified timeout parameter to TCP sockets #245

Closed
shamblett opened this issue Feb 7, 2021 · 13 comments
Closed

Add a user specified timeout parameter to TCP sockets #245

shamblett opened this issue Feb 7, 2021 · 13 comments

Comments

@shamblett
Copy link
Owner

TCP sockets allow a timeout value to be passed on connect -

Socket.connect(server, port, timeout: new Duration(seconds: 5)).then((dynamic socket) 

This could be added as a parameter to the server client.

@jonny7737
Copy link

jonny7737 commented Feb 28, 2023

EDIT: After further testing, this only works on the initial connection. Auto reconnects do not honor this setup. Looking deeper I see that connectAuto must be modified also. That works.

Temporery solution only. A hack not a fix.

FWIW: the timeout parameter of the Socket.connect() call is only a timeout for the connect sequence. If the connection is not made before the timeout occurs... An idle timeout on an active connection requires a little more configuration.

I modified mqtt_client_mqtt_server_normal_connection: connect override: add this code at the top of the Socket.connect().then(){} section

        final enableKeepaliveOption = RawSocketOption.fromBool(
            0xffff, // SOL_SOCKET
            0x0008, // SO_KEEPALIVE
            true
        );
        final keepaliveIntervalOption = RawSocketOption.fromInt(
            6, // IPPROTO_TCP
            0x10, // TCP_KEEPALIVE
            2
        );
        final keepaliveSuccessiveIntervalOption = RawSocketOption.fromInt(
          6, // IPPROTO_TCP
          0x101, // TCP_KEEPINTVL
          1,
        );
        final keepaliveSuccessiveCountOption = RawSocketOption.fromInt(
            6, // IPPROTO_TCP
            0x102, // TCP_KEEPCNT
            2
        );
        socket.setRawOption(enableKeepaliveOption);
        socket.setRawOption(keepaliveIntervalOption);
        socket.setRawOption(keepaliveSuccessiveIntervalOption);
        socket.setRawOption(keepaliveSuccessiveCountOption);

FOR MACOS: this code addition provides the expected timeout when the server abruptly disconnects from the network.
This solution is not mine and I have no details on the meaning of the values used to create the RawSocketOption objects.

@shamblett
Copy link
Owner Author

I didn't think to check the raw socket options, thanks for this, I'll update both packages with this and re release them.

@shamblett
Copy link
Owner Author

Hmm, a few unit test are now failing with 'OS Error: Protocol not available, errno = 92', the mqtt_client_connection_unsecure_test.dart is an example, I'll dig deeper into whats happening here.

@jonny7737
Copy link

jonny7737 commented Mar 1, 2023

In what environment was the failed test run? If other than macos, then the parameters for the RawSocketOption.from... constructor are the likely cause. These values are different for Android.

I see no such failures when running my app on macos.

@shamblett
Copy link
Owner Author

It fails on my main linux dev box which runs Fedora 37 as it happens,

@jonny7737
Copy link

jonny7737 commented Mar 2, 2023

My Debian VM defins the TCP_keys as:

#define TCP_KEEPIDLE		4	/* Start keeplives after this period */
#define TCP_KEEPINTVL		5	/* Interval between keepalives */
#define TCP_KEEPCNT		6	/* Number of keepalives before death */

Hope this is still helping.

@shamblett
Copy link
Owner Author

Just quickly commenting out the setting of the options and seeing what happens has revealed that setting any of the options causes the problem, even if you substitute the values from Linux then you would also need values for windows and arm and ultimately RISCV. This will get to messy and will need platform specific code which I don't want to add.

So, I think the best thing to do is to put a generic socket option API on the client, so the user can set up the values they want using addBool, addInt etc. and the client will apply these on connect. This way if it breaks it can easily be undone, the user is free to set any socket option they like on whatever platform they like.

What do you think?

@jonny7737
Copy link

jonny7737 commented Mar 2, 2023 via email

@shamblett
Copy link
Owner Author

OK, Ill create a branch for this and add it to the client, shouldn't take too long.

@shamblett
Copy link
Owner Author

OK, I've updated the server client to accept a list of user supplied RawSocketOptions, if you could point your pubspec at the mqtt_client repo and set the branch to issue245 and look at the ' socketOptions' API it would be good to get some feeback from you before I re publish the package, thanks.

@jonny7737
Copy link

jonny7737 commented Mar 4, 2023

Simple, easy and works as expected. Thank you so much.

if(Platform.isMacOS) {  
    final enableKeepaliveOption = RawSocketOption.fromBool(0xffff, 0x0008, true);
    final keepaliveIntervalOption = RawSocketOption.fromInt(6, 0x10, 2);
    final keepaliveSuccessiveIntervalOption = RawSocketOption.fromInt(6, 0x101, 1);
    final keepaliveSuccessiveCountOption = RawSocketOption.fromInt(6, 0x102, 1);

    client.socketOptions = [
      enableKeepaliveOption,
      keepaliveIntervalOption,
      keepaliveSuccessiveIntervalOption,
      keepaliveSuccessiveCountOption
    ];
}

@jonny7737
Copy link

1 out of approx. 50 tests of broker abruptly terminating showed an unhandled exception. Not a problem i think but wanted to let you know just in case:

[ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: SocketException: Write failed (OS Error: Connection reset by peer, errno = 54), address = 10.211.55.8, port = 53154

#0      _NativeSocket.write (dart:io-patch/socket_patch.dart:1201:34)
#1      _RawSocket.write (dart:io-patch/socket_patch.dart:1944:15)
#2      _Socket._write (dart:io-patch/socket_patch.dart:2387:18)
#3      _SocketStreamConsumer.write (dart:io-patch/socket_patch.dart:2134:26)
#4      _SocketStreamConsumer.addStream.<anonymous closure> (dart:io-patch/socket_patch.dart:2108:11)
#5      _RootZone.runUnaryGuarded (dart:async/zone.dart:1593:10)
#6      _BufferingStreamSubscription._sendData (dart:async/stream_impl.dart:339:11)
#7      _BufferingStreamSubscription._add (dart:async/stream_impl.dart:271:7)
#8      _SyncStreamControllerDispatch._sendData (dart:async/stream_controller.dart:774:19)
#9      _StreamController._add (dart:async/stream_controller.dart:648:7)
#10     _StreamController.add (dart:async/stream_controller.dart:596:5)
#11     _StreamSinkImpl.add (dart:io/io_sink.dart:136:17)
#12     _Socket.add (dart:io-patch/socket_patch.dart:2233:38)
#13     MqttServerNormalConnection.send (package:mqtt_client/src/connectionhandling/server/mqtt_client_mqtt_server_normal_connection.dart:104:13)
#14     MqttConnectionHandlerBase.sendMessage (package:mqtt_client/src/connectionhandling/mqtt_client_mqtt_connection_handler_base.dart:167:18)
#15     MqttConnectionKeepAlive.pingRequired (package:mqtt_client/src/connectionhandling/mqtt_client_mqtt_connection_keep_alive.dart:89:28)
#16     Timer._createTimer.<anonymous closure> (dart:async-patch/timer_patch.dart:18:15)
#17     _Timer._runTimers (dart:isolate-patch/timer_impl.dart:398:19)
#18     _Timer._handleMessage (dart:isolate-patch/timer_impl.dart:429:5)
#19     _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:192:26)

@shamblett
Copy link
Owner Author

In your use case I think this is acceptable but thanks for the research anyway.

I've merged the change and re published the package at version 9.8.0, also I've raised this issue on the mqtt5_client to update it also.

Closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants