WiFiClient - assignment shouldn't stop connection

[JAndrassy]

assignment shouldn't stop connection in the left client. there may be other copy of WiFiClient working with that connection.
let shared_ptr stop the connection when it is not referred anymore.
the default assignment operator works ok.

[github-actions]

Warnings

⚠️

Some issues found for the commit messages in this PR: the commit message "WiFiClient - assignment shouldn't stop connection": summary looks empty type/action looks empty Please fix these commit messages - here are some basic tips: follow Conventional Commits style correct format of commit message should be: <type/action>(<scope/component>): <summary>, for example fix(esp32): Fixed startup timeout issue allowed types are: change,ci,docs,feat,fix,refactor,remove,revert,test sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter avoid Jira references in commit messages (unavailable/irrelevant for our customers) TIP: Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).

👋 Hello JAndrassy, we appreciate your contribution to this project!

---

Click to see more instructions ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against 368648b

[VojtechBartoska]

Same here, thanks for the PR, awaiting review.

[lucasssvaz]

LGTM

[TD-er]

In the current implementation, the call to stop() does clear the last read and write timeout.

arduino-esp32/libraries/WiFi/src/WiFiClient.cpp

Lines 200 to 207 in cc6da05

	void WiFiClient::stop()
	{
	clientSocketHandle = NULL;
	_rxBuffer = NULL;
	_connected = false;
	_lastReadTimeout = 0;
	_lastWriteTimeout = 0;
	}

Those are not copied from the other WiFiClient. (in the current situation)
No idea if Client from which WiFiClient inherits has any members which need to be copied, but those are also not copied.

No idea yet whether that's important or not.

In your PR you let the compiler generate this function , which essentially does the same as the stop() call except for the reset of the timeout members.

I can't oversee yet whether this is a problem, but what I do see is that this change does not match your description of why you need this change as it still does reassign a clientSocketHandle and buffer.

The only thing I can imagine is that maybe the shared_ptr count does not reach 0 when you assign it to itself? (or indirectly assign it to itself)

So my question is what actual problem or use case do you try to fix here?

[JAndrassy]

So my question is what actual problem or use case do you try to fix here?

the actual problem is a warning (which causes PR tests to end with error). the warning says that a copy constructor must exist if assignment operator is defined. but the assignment operator is only defined to stop the connection. and it shouldn't do that. so instead of adding a copy constructor we delete the assignment operator.

[TD-er]

Sounds like you're also testing using PVS Studio? :)

Or is there a compiler flag which can also tell me this, then I would really like to know (and use) it. :)

Anyway, about this PR, I don't know whether this actually may change behavior as it depends on the implementation of the shared_ptr and what these timeout values are used for.

[JAndrassy]

I use Eclipse Sloeber.
The default copy constructor and assignment operator are well defined. They copy all fields the same way parameters of a function are copied.
shared_ptr is designed exactly for this case. to count copies and delete the managed object when the last copy is destructed.

[TD-er]

shared_ptr is designed exactly for this case. to count copies and delete the managed object when the last copy is destructed.

I know, but like I said the only difference here is that the shared_ptr is assigned a nullptr inbetween (current implementation with call to stop().
Thus if you are not assigning the same connection to it, then it makes no difference whether you call stop() inbetween or not. (apart from the already mentioned differences of not copying the timeout vars)
In either case any reference count to the existing connection is decreased by 1.

So the only difference here is there will be an assignment of a nullptr inbetween. (current implementation)
Now that you're removing this extra assignment, the only use case I can imagine which will be different is when you're assigning the same connection to this WiFiClient.
If the assignment operator of shared_ptr does check whether it already has the same pointer address, then there is a change in behavior.
If it doesn't, then it is exactly the same behavior (regarding the shared_ptr variables) as before.
In any other use case I don't see how you're preventing the current connection to be closed when assigning the other client. (N.B. the connection of other is not closed)

[JAndrassy]

sorry. yes. I forgot that in esp8266 the connection closes from local side only if the last copy is destroyed. that is a way to prevent invalid copies. already two or three time in last years I wondered why a connection continuous to work when I invoked stop()

[me-no-dev]

@JAndrassy so what does this PR improve in reality?

[JAndrassy]

so what does this PR improve in reality?

removes code which causes trouble and confusion. for example the WiFiServer PR #9028 fails checks because of existence of this assignment operator

[me-no-dev]

PR is failing, because you are compiling for H2, which does not have WiFi at all. But yes, this also gives warning.