Add @nogc attribute to Socket receive methods

[pinver]

ditto

[dlang-bot]

Thanks for your pull request and interest in making D better, @pinver! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	19311	enhancement	Add @nogc attribute to std.socket receive methods

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#6730"

[n8sh]

In the future the commit message should refer to an issue number in the bug tracker. I've added it for this one.

[pinver]

You are right, sorry.

I'll open an issue the next time.

[CyberShadow]

Unfortunately, because Socket is not a final class, this change will break code like this:

import std.socket;

class MySocket : Socket
{
	override ptrdiff_t receive(void[] buf)
	{
		new int[1000];
		return 0;
	}
}

There is a test to check against wrong attributes lower, but it looks like it hasn't triggered because the compiler is inferring @nogc from the method body.

Also, as a whole, std.socket is very much non-@nogc, so this change seems rather moot.

[pinver]

Understood: Is it acceptable to add to Socket something like receive_nogc?

I understand that as a whole, std.socket is very much non-@nogc, but there's no way right now to stick a receive into a @nogc stack of processing functions.

As a precedent, something similar is present in Mutex, with lock_nothrow.

[CyberShadow]

Is it acceptable to add to Socket something like receive_nogc?

Not sure I can give a good answer to that. Generally, std.socket is stuck in an old, D1-esque design, and the community responded to this by forking or writing their own socket libraries on top of the libc / OS primitives. There's definitely a lot of things that could be improved than making a few spots @nogc, so it would have to be part of something more orchestrated and less ad-hoc.

I understand that as a whole, std.socket is very much non-@nogc, but there's no way right now to stick a receive into a @nogc stack of processing functions.

What's special about receive? What about send and other methods?

[pinver]

Not sure I can give a good answer to that. Generally, std.socket is stuck in an old, D1-esque
design, and the community responded to this by forking or writing their own socket libraries on
top of the libc / OS primitives.

Yes, I know that, but I would prefer to stick with plain old Phobos, for a series of reasons...

There's definitely a lot of things that could be improved than making a few spots @nogc,
so it would have to be part of something more orchestrated and less ad-hoc.

What have you in mind as more orchestrated and less ad-hoc? I'm out of ideas about alternatives, if we can't stick a @nogc to the actual signature.

What's special about receive?

Well, nothing.... actually the use case that triggered the problems was in a receiver process.

What about send and other methods?

I just want to keep the pull request small... it started as a small @nogc addition after all... :-P

[CyberShadow]

What have you in mind as more orchestrated and less ad-hoc? I'm out of ideas about alternatives, if we can't stick a @nogc to the actual signature.

Before we lock ourselves into a local maximum by extending the public API while trying to fix this one particular problem, we should consider previous similar problems encountered in the past:

• std.socket: Fix some -dip1000 compilable issues #6204
• Fix forced non-const Address #5496
• std.socket: Decorate Socket methods #4273

Perhaps one approach would be a final descendant of Socket which has the strictest possible attributes of its implementation.

[pinver]

Before we lock ourselves into a local maximum by extending the public API while trying to fix
this one particular problem, we should consider previous similar problems encountered
in the past:

• std.socket: Fix some -dip1000 compilable issues #6204
• Fix forced non-const Address #5496
• std.socket: Decorate Socket methods #4273

Ok, I've read the references, it seems that for the foreseeable future we are pretty stuck to the actual situation.

Perhaps one approach would be a final descendant of Socket which has the strictest
possible attributes of its implementation.

Casting in the descendant to use the base methods, do you mean?

What about adding a new final method with a different name to the actual socket, with the strictest possible attributes of its implementation, and forward and deprecate the old one to that?

[CyberShadow]

Casting in the descendant to use the base methods, do you mean?

Yep.

What about adding a new final method with a different name to the actual socket, with the strictest possible attributes of its implementation, and forward and deprecate the old one to that?

No good. It would make a type system hole - non-@nogc implementations in some user class descending from Socket would then be accessible from @nogc code via the new wrappers in the base Socket class.

[pinver]

Casting in the descendant to use the base methods, do you mean?

Yep.

If we cast, we must tag every new final method as @trusted also if the implementation is the original method could be marked as @safe.

That would turn code that can be mechanically checked as @safe to code that must be checked by humans, does this worths?

What about adding a new final method with a different name to the actual socket, with the strictest possible attributes of its implementation, and forward and deprecate the old one to that?

No good. It would make a type system hole - non-@nogc implementations in some user class descending from Socket would then be accessible from @nogc code via the new wrappers in the base Socket class.

The other way round: the old non-@nogc method implementation would be forward to the new @nogc and final method of the same class.

Derived classes can only override the old non-@nogc methods, without breaking nothing, and new code can be written against the new final @nogc methods doing the real works.

[Geod24]

@pinver : In your use case, you might be able to just define a class Socket : std.socket.Socket type in your code which override those methods and make them @nogc, since in D, it's legal for a derived class to add stricter attributes.
Unless you are using foreign code that returns an std.socket.Socket, in which case there's going to be some rough edge.

[CyberShadow]

If we cast, we must tag every new final method as @trusted also if the implementation is the original method could be marked as @safe.

A private trusted wrapper would work too, but I agree it's better to have the compiler infer the attributes from the implementation raher than ensure by hand it matches up.

The other way round: the old non-@nogc method implementation would be forward to the new @nogc and final method of the same class.

Ah, yes, that would work. It would be annoying to have to define/use alternate names for all the methods, though. Maybe put the final implementations into a new superclass that Socket descends from, and provides virtual wrappers for those implementations?

In any case, new additions would have to be reviewed by @andralex, maybe he will have a better idea.

[pinver]

@pinver : In your use case, you might be able to just define a class Socket : std.socket.Socket type in your code which override those methods and make them @nogc, since in D, it's legal for a derived class to add stricter attributes.
Unless you are using foreign code that returns an std.socket.Socket, in which case there's going to be some rough edge.

Thanks Mathias, if I'm not wrong, you still need to cast:

class NoGcSocket : Socket {
    @safe override ptrdiff_t receive(void[] but) @nogc { return super.receive(buf); }
}
onlineapp.d(6): Error: @nogc function onlineapp.NoGcSocket.receive cannot call non-@nogc function std.socket.Socket.receive

That leads to the introduction of @trusted methods when in the base are @safe, and that's, IMHO, suboptimal.

[Geod24]

If you call the base method, yes. It's not a general solution by any means. But in this case I would just copy the code and call it a day.

[pinver]

Ah, yes, that would work. It would be annoying to have to define/use alternate names for all the methods, though.

You are right, but on the other side, that pattern was already used in the past, for example in core.sync.mutex

Maybe put the final implementations into a new superclass that Socket descends from, and provides virtual wrappers for those implementations?

My humble opinion is just to stick with minimal changes: the hot path in socket usage is bound to receive/send, where latency can be a problem (and that's my case). Usually it's not a problem the initialisation phase of the socket itself, bind, etc that can be done also by non-@nogc methods as the actual one.

Adding a stricter method to the actual socket class seems to me the best way to proceed, with the minimum impact, also taking in account that we have the perspective of a better global refectory of IO with Martin std.io.

Let's wait for Andrei.

[pinver]

If you call the base method, yes. It's not a general solution by any means. But in this case I would just copy the code and call it a day.

If I'm following your reasoning correctly, I can't do that, at least in an external module, as the needed data member socket is private:

class Socket
private:
    socket_t sock;
    <snip>

[atilaneves]

What's special about receive? What about send and other methods?

receive allocates, send never has to.

[pinver]

@atilaneves , what is the best way to move forward on that? It would be great if we can define a 'generic' policy for other similar PR against Phobos.

[atilaneves]

I'm not sure a generic policy would work, but in this case I'd say that given the potential breakage of child classes that we shouldn't merge. A Phobos v2 would then have @nogc member functions. I don't even know why they'd even be classes.

[pinver]

Thank you Atila,
Have you taken in account the possibility of adding a receive_nogc method along with keeping the current one?
As a precedent, something similar is present in Mutex, with lock_nothrow.

The new old receive would simply forward to the old one, so that should not be a maintenance burden either.

[atilaneves]

I'm not a fan of receive_nogc. If I were you I'd copy the code from Phobos and slap @nogc on the member functions. And while at it change them to structs.

[pinver]

Thank you for your advice, we did something similar for our customer code: but I think the point here is on Phobos.

Feel free to correct me if I'm speculating wrongly, but I've understood that your opinion is that the module should be rewritten / reorganised deeply (struct vs class for ex), potentially in Phobos V2, and that's fine.

At the same time, while waiting, we are unable to write @nogc code in a hot IO path without recurring to @trusted. Reviewers simply asked us why, and why we needed to use custom code for something that simple, and not the consolidated, audited, standard library code.

What are the drawback in adding a single, better method to a codebase that's should be rewritten? Can you elaborate further? In the discussion is pretty clear that the burden added to maintenance is really low.

I don't want to press you too much on this, so this is my last attempt, then I will conform to your judgement as gatekeeper.

[atilaneves]

@pinver I see. I guess the least bad option is a separate member function then.

[RazvanN7]

So, how should we move further here?

[atilaneves]

receive_nogc or receiveNogc.

[RazvanN7]

ping @pinver

[pinver]

Thank you all for your feedback, that's great!

Actually I'm all-in in time-constrained mode ...
Razvan, if it's not a problem, please keep this PR open, I'll try to come back to this ASAP ...

[RazvanN7]

@pinver is this still on your radar?

[RazvanN7]

@pinver I will close this for now, but please feel free to reopen and ping me whenever you have made some changes.