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

Defer websocket connect() #889

Closed
wants to merge 3 commits into from

Conversation

Pietfried
Copy link
Contributor

@Pietfried Pietfried commented Nov 28, 2024

Describe your changes

The start() function of the charge_point could be blocking until a connection timeout occured when connection the websocket for the first time. This change defers the call to the connect() function so that the start() function does not block anymore.

Additionally introduced thread_mutex protecting access to threads of libwebsockets when the ~WebsocketLibwebsockets destructor was called it attempted to acquire a lock for the connection_mutex and joins the websocket_thread . But the websocket_thread runs the client_loop that also requires the lock when calling on_conn_fail, so the client_loop which could result in a deadlock

Issue ticket number and link

Checklist before requesting a review

marcemmers and others added 2 commits November 28, 2024 13:22
…ion for OCPP1.6 ; Removed WEBSOCKET_INIT_DELAY on startup

Signed-off-by: Piet Gömpel <[email protected]>
@@ -135,7 +135,7 @@ void ConnectivityManager::connect(std::optional<int32_t> configuration_slot_opt)
// After the websocket gets closed a reconnect will be triggered
this->websocket->disconnect(WebsocketCloseReason::ServiceRestart);
} else {
this->try_connect_websocket();
this->websocket_timer.timeout([this] { this->try_connect_websocket(); }, std::chrono::seconds(0));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcemmers I used std::chrono::seconds(0) instead of WEBSOCKET_INIT_DELAY because the WEBSOCKET_INIT_DELAY was only introduced to solve an issue with OCTT tests reconnects as far as I remember.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I don't think there would be a point delaying there. Although I think we should probably revisit misusing a timer for this at all, that would be out of scope for this.

* Introduced thread_mutex protecting access to threads of libwebsockets when the ~WebsocketLibwebsockets destructor was called it attempted to acquire a lock for the connection_mutex and joins the websocket_thread . But the websocket_thread runs the client_loop that also requires the lock when calling on_conn_fail, so the client_loop which could result in a deadlock

Signed-off-by: Piet Gömpel <[email protected]>
@Pietfried
Copy link
Contributor Author

@marcemmers please have another looks at the recent changes

@Pietfried
Copy link
Contributor Author

@marcemmers please have another looks at the recent changes

Hi @marcemmers , we are looking into it and considering some more changes to the WebsocketLibwebsockets class, so you can wait with the review for this one

@Pietfried
Copy link
Contributor Author

Closed in favor of #896

@Pietfried Pietfried closed this Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants