mochiweb_http:start/1 effectively links #58

Closed
yrashk opened this Issue Aug 30, 2011 · 7 comments

Projects

None yet

2 participants

@yrashk
yrashk commented Aug 30, 2011

https://github.com/mochi/mochiweb/blob/master/src/mochiweb_socket_server.erl#L128

IMHO, start/* is expected not to link, if somebody wants to link, there should be a separate start_link/* function.

@etrepum
Member
etrepum commented Aug 30, 2011

What's the use case for having start not link? Do you have a proposal to do this in a backwards compatible way?

@yrashk
yrashk commented Aug 30, 2011

My use case was when I need to try starting mochiweb, but if its returning {error, eaddrinuse}, do something else. start/1 tricked me into believing that it does not link to the current process. Since it did, it actually crashed my gen_server, so I had to resort to gen_tcp:listen to try port's availability first.

Besides, it's just breaking the convention that start doesn't link but start_link does. I don't think this adjustment can be done in a backwards compatible way.

One option that I see is to add {link, boolean()} option to disable linking when necessary. It will not solve the problem of figuring out that start does something it is not supposed to do, but at least it will provide a way to get around of that behaviour.

@etrepum
Member
etrepum commented Aug 30, 2011

I agree that it would be ideal that start did not link, but backwards compatibility is a more important goal for us right now. The mistake was made several years ago, LOTS of code uses mochiweb (most of which we don't have any control over), and this is the first request to change that behavior to my knowledge.

Perhaps we could add a start_link, start/2, and add a deprecation message to start/1 or something like that. Definitely not something we could just fix very quickly.

@yrashk
yrashk commented Aug 30, 2011

start_link plus an option for using start in a way that it won't link ({link, false} option) will be great. Plus a documentation saying that until this is deprecated, start actually does link so people can have easier time figuring this out.

@etrepum
Member
etrepum commented Aug 30, 2011

So I think I'm going to add a start_link/1 where appropriate and add a {link, false} option to start/1 with no deprecation warnings yet. I'll leave some comments in the code so that we know how to phase out the linked start/1 behavior carefully (will take a while to do it smoothly: add {link, false} and start_link/1, deprecation of start/1 without {link, false}, change default behavior, add deprecation warning for {link, false}, removal of {link, false}).

@etrepum
Member
etrepum commented Aug 30, 2011

I think this should solve your request for now, with the expectation that we'll eventually phase out the current (stupid) default. Please re-open if you think something else needs to change.

fb4a292

@etrepum etrepum closed this Aug 30, 2011
@yrashk
yrashk commented Aug 30, 2011

thanks a lot! that was really quick :)

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