Update ssl.py

[RuitongZhu]

replace hard coded class name to avoid inconvenience when inheriting from this class

[temoto]

please describe problem fixed by this patch

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d78f8f6) 53% compared to head (9369404) 53%.
Report is 6 commits behind head on master.

Additional details and impacted files

@@          Coverage Diff          @@
##           master   #796   +/-   ##
=====================================
  Coverage      53%    53%           
=====================================
  Files          88     88           
  Lines        9848   9848           
  Branches     1841   1841           
=====================================
+ Hits         5299   5300    +1     
+ Misses       4164   4163    -1     
  Partials      385    385           

Flag	Coverage Δ
ipv6	22% <0%> (ø)
py310epolls	52% <100%> (+<1%)	⬆️
py310poll	52% <100%> (-1%)	⬇️
py310selects	52% <100%> (ø)
py311epolls	52% <100%> (+<1%)	⬆️
py38epolls	52% <100%> (-1%)	⬇️
py38openssl	50% <100%> (+<1%)	⬆️
py38poll	52% <100%> (ø)
py38selects	51% <100%> (+<1%)	⬆️
py39dnspython1	50% <100%> (-1%)	⬇️
py39epolls	52% <100%> (+<1%)	⬆️
py39poll	52% <100%> (+<1%)	⬆️
py39selects	51% <100%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[damani42]

Looks good to merge, but I agree with @temoto it will be great to have more detail. Please @RuitongZhu can five more information?

[4383]

I think I get the point but I'm not sure this change is really needed... so yes, more details would be welcome.

I think @RuitongZhu's thougths, followed the of scenario below.

First, __class__ is a reference to the type of the current instance.

When creating a new instance of GreenSSLSocket, __class__ is hard coded with the current class name GreenSSLSocket.

So, if an eventlet user want to inherit from GreenSSLSocket , and then check the instance of its own object, the user will get compare its object to a GreenSSLSocket type (the type of parent class).

However, that's not what we get.

Consider the sample below which lead more or less to the same scenario:

class A:
    def new(cls):
        cls.__class__ = A
        return cls

class B(A):
    pass


if __name__ == "__main__":
    a = A()
    b = B()
    print(isinstance(a, A))
    print(isinstance(b, A))
    print(isinstance(b, B))
    print(type(a))
    print(type(b))

Then we obtain the following output:

True
True
True
<class '__main__.A'>
<class '__main__.B'>

So even if __class__ is hard coded by A Python is enough clever to see that b is also an instance of both B and A.

My words are just guessing about the thoughts of the author of this pull request, maybe I'm wrong.

[4383]

@RuitongZhu: Any feedback to share with us?

[RuitongZhu]

Sorry for the late reply. It's been quite a while... I've left my previous company and lost context of the request.
But I can try and argue for the change.
From the code example @4383 provided, the function new() was never really called. Thus the code example does not make much sense.
The class assignment for GreenSSLSocket was in the construction method __new__().
If you call the new() function like below:

if __name__ == "__main__":
    a = A()
    b = B()
    b.new()
    print(isinstance(a, A))
    print(isinstance(b, A))
    print(isinstance(b, B))
    print(type(a))
    print(type(b))

You would get result:

True
True
False
<class '__main__.A'>
<class '__main__.A'>

[4383]

Sorry for the late reply. It's been quite a while... I've left my previous company and lost context of the request. But I can try and argue for the change. From the code example @4383 provided, the function new() was never really called. Thus the code example does not make much sense. The class assignment for GreenSSLSocket was in the construction method __new__(). If you call the new() function like below:

My bad, that's a typo, I wanted to wrote __new__( instead of new(.

However, I modified my previous example to:

1. fix the __new__ issue
2. better match the original use case

Here is the code:

class AnotherClass:
    pass


class A:
    def __new__(cls):
        ret = AnotherClass()
        ret.__class__ = cls
        return ret


class B(A):
    pass


if __name__ == "__main__":
    a = A()
    b = B()
    print(isinstance(a, A))
    print(isinstance(b, A))
    print(isinstance(b, B))
    print(type(a))
    print(type(b))

And even with that I still obtain the right result:

True
True
True
<class '__main__.A'>
<class '__main__.B'>

[RuitongZhu]

Hi @4383 , note that in your example you did not hard code the __class__ but use cls instead. That's exactly what my change was trying to do.
To illustrate the current behavior, the code shall be modified as:

class A:
    def __new__(cls):
        ret = AnotherClass()
        ret.__class__ = A
        return ret

and the result would be

True
True
False
<class '__main__.A'>
<class '__main__.A'>

[4383]

Indeed, I confirm that by replacing ret.__class__ = cls by ret.__class__ = A in my previous example, I'm now able to reproduce this use case.

I think we should also consider the following checks to better consider this change request:

a = A()
b = B()
print(isinstance(a, A))
print(isinstance(a, AnotherClass))
print(isinstance(b, A))
print(isinstance(b, B))
print(isinstance(b, AnotherClass))
print(type(a))
print(type(b))

Previous code will output:

True
False
True
False
False
<class '__main__.A'>
<class '__main__.A'>

[4383]

I don't know if this behaviour is something volunteer or not.

@RuitongZhu: From your perspective why the current behaviour is an issue? Do you remember your use case?

I'm not against merging these changes, however, now, that I'm able to reproduce, and now that changed my mind concerning my intial comment, I need to better understand your use case. I want to avoid side effect to anyone else.

[RuitongZhu]

I don't know if this behaviour is something volunteer or not.

@RuitongZhu: From your perspective why the current behaviour is an issue? Do you remember your use case?

I'm not against merging these changes, however, now, that I'm able to reproduce, and now that changed my mind concerning my intial comment, I need to better understand your use case. I want to avoid side effect to anyone else.

I think it's already pretty clear from the example. For a wrapper class GreenSSLSocketWrapper to inherit from GreenSSLSocket, it has to overwrite the __new__ method since __class__ was hard coded to be GreenSSLSocket. The trouble could be saved if cis was used.

[4383]

I don't know if this behaviour is something volunteer or not.
@RuitongZhu: From your perspective why the current behaviour is an issue? Do you remember your use case?
I'm not against merging these changes, however, now, that I'm able to reproduce, and now that changed my mind concerning my intial comment, I need to better understand your use case. I want to avoid side effect to anyone else.

I think it's already pretty clear from the example. For a wrapper class GreenSSLSocketWrapper to inherit from GreenSSLSocket, it has to overwrite the __new__ method since __class__ was hard coded to be GreenSSLSocket. The trouble could be saved if cis was used.

Got it.
Thanks for the details and for the discussion.

[4383]

Considering the details given in the discussion, these changes LGTM.