Closed Bug 92928 Opened 23 years ago Closed 21 years ago

Server socket support in Necko

Categories

(Core :: Networking, enhancement)

Product:
Core
Component:
Networking
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.6beta

People

(Reporter: daniel, Assigned: darin.moz)

References

Details

Attachments

(4 files, 4 obsolete files)

implementation proposal (for information see above)
7.85 KB, application/octet-stream
Details
Resent same attachment as id 44107, but correct MIME type "application/zip" (Thanks Boris)
7.85 KB, application/zip
Details
Implementation proposal
36.77 KB, patch
Details | Diff | Splinter Review
v2.1 patch
38.72 KB, patch
dougt
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
I'm currently implementing server socket support in Necko integrated into the nsISocketTransport object. A new method called "asyncListen" is introduced, which takes a newly created "nsISocketListener" interface as a parameter. The socket used to call the method is switched into listening mode and is put in the work queue of nsISocketTransportService. Everytime a client application connects to this socket, the socket is accepted and the listener interface's "onSocketAccepted" method is called with the accepted socket as a parameter. With this support it should be possible to realize at least the following things: 1) P2P servent (that's why I need it) 2) PORT command implementation during FTP session with servers that are not PASV-capable 3) ident server (see bug #40879) If anybody has any comments to this idea and/or its implementation I'd like to hear from her/him. If this is considered useful, perhaps it gets part of the necko core (which I would appreciate because the lack of this feature regularly was/is subject in the netlib newsgroup).
ccing some networking folks and adding dependencies. The attachment looks like a zip file... perhaps it would be better to attach such things as application/zip
Blocks: 40879, 74045
Status: UNCONFIRMED → NEW
Ever confirmed: true
*** Bug 74045 has been marked as a duplicate of this bug. ***
This would make DCC chat and file transfers possible as well, I'm all for it!
Blocks: 73257
No longer blocks: 74045
Daniel, what format is the attachment in?
at first glance, the patch looks good. however, it's unlikely to make it in for 0.9.4 (as checkins are going to be tightly controlled and limited in scope). i'll try to do a more thorough review soon (but no promises ;-)
also blocks bug 465.
Blocks: 465
darin: 0.9.4 is out... :) (I haven't looked at the patch)
Keywords: patch, review
please reattach the patch as a diff.. thanks!
Not that you don't already have enough reasons, but this would also allow 2player network games (why I need it). Gets my vote :)
nevermind my request to see the whole diff as a single patch... i didn't realize that that would require CVS write access :-( at any rate, there is definitely work to be done on the patch as outlined in the newsgroup thread on n.p.m.netlib "Re: Bug 92928 (Server sockets): New DIFF"
COPY OF MESSAGE FROM n.p.m.netlib: ================================== Hi, > the nsIServerSocket would still need to participate in the same > PR_Poll loop, which would mean changes to nsSocketTransportService to > work with something other than just nsSocketTransport objects. one possible (very object oriented) approach to this would be to use a common base class, that implements just enough to work as a layer between the nsSocketTransportService and objects that are part of the work queue. An example is the class nsSocketTransportServiceLayer located in the nsSocketTransportService.h after applying the patch attached to this mail. Although this class inherits nsISupports it should not be available by calling QueryInterface because the methods are too implementation dependend (I don't know if this proceeding is acceptable or do we rely on noboby calling the contained functions???). For this reason the interface has no IID and the member functions don't follow the calling conventions for interface methods. You get a pointer to this layer interface using a static cast on the super class. The nsISupports is only necessary for the reference counting during the work queue processing. Both classes nsSocketTransport and the new nsServerSocket inherit of this class and implement the pure virtual functions. This already works fine for the nsSocketTransport class except for the methods using a static cast to convert a nsITransport pointer into a nsSocketTransport pointer (this looks a bit critical...) One of them is nsSocketTransportService::Wakeup().The original implementation uses the static cast to call nsSocketTransportService::AddToWorkQ afterwards. The patch's implementation relies on the fact, that the transport passed to the Wakeup method has to be in the selection list of nsSocketTransportService, otherwise a wakeup would not make any sense. So it traverses all selection list entries to find the nsSocketTransportServiceLayer that belongs to the i_Transport parameter. In the worst case that means MAX_OPEN_CONNECTIONS loop passes. But since this function is rarely used I think this is okay... Other opinions??? The second (and more tricky) method is nsSocketTransportService::ReuseTransport(). In this case I cannot assume the requested transport to be in the selection list. The most simple way (but not my goal, see above) would be to let QueryInterface return the nsSocketTransportServiceLayer (or a nsISocketTransportServiceLayer respectively) and call its methods... Suggestions welcome! If you agree with this basic approach I would start implementing the server socket support as well. Thanks, Daniel
Attached patch Patch referring to comment above (obsolete) — — Splinter Review
using a common base class for server sockets and socket transports is the right thing IMO. Wakeup should be modified to accept a nsSocketTransportServerLayer (btw, this really could use a shorter name!), and ReuseTransport should QI the given i_Transport for nsISocketTransport. if the QI succeeds, then you can just STATIC_CAST to nsSocketTransport and call CanBeReused. on the other hand, no one is currently calling ReuseTransport, and IMO it should simply be eliminated in favor of a method on nsISocketTransport.
1) "nsSocketTransportServerLayer (btw, this really could use a shorter name!)": nsSocketTransportServiceLayer => nsCommonSocketLayer or just nsSocketLayer ??? 2) "Wakeup should be modified to accept a nsSocketTransportServerLayer": Should Wakeup take a pointer to a nsSocketTransportServiceLayer object as a parameter (which makes no sense to me) or do you mean "modified" in the way the patch does?
Wakeup should not be a method on nsISocketTransportService... it should be a method on nsSocketTransportService that nsSocketTransport can call. there is no need for us to expose it (as well as ReuseTransport) from nsISocketTransportService. if we remove these functions from the .idl file, then Wakeup could be modified to take a nsSocketTransportServiceLayer parameter, and ReuseTransport could be removed completely.
maybe nsSTSLayer? i like nsSocketTransportServiceLayer because it correctly describes what it is, but it would just be nice if it were shorter... i don't know about nsCommonSocketLayer or nsSocketLayer... seems too vague... since "socket" shows up elsewhere in the code as part of class names.
Implementation proposal based on attachment 57617 [details] [diff] [review] and this topic's discussions (incl. thread on n.p.m.netlib). Opinions and suggestions welcome :-) Daniel
Will bug 465 be reopened if this gets fixed? This is currently my top blocker. I need port support. I know most broadband is firewalled thus passive mode is popular, but... Weirdly my broadband requires port :(
It is hard to say given the problems with FTP PORT. I'm sure we will have a lengthy discussion and make a good decision.
It would be really great if this bug were fixed by moz 1.0 - I know of several applications for this - one of which I am working on myself. It seems like this could really enhance the range of applications that mozilla could potentially be used for - such as IRC server, peer to peer, file transfer in AIM, etc. etc.
Why on Earth would you want to run an IRC *server* as part of Mozilla?
gwalla: Necko can be used in more places than just mozilla the browser no? Besides DCC and ident for chatzilla needs it.
> Necko can be used in more places than just mozilla the browser no? True. Just wondering why anyone would want to base an IRC server on necko, that's all. DCC is another thing entirely (and why I'm following this bug).
daniel: i've looked over the patch and one problem i see right off the bat is that the indentation is all wacked. be sure that indentation does not consist of tab characters, as that is required by the mozilla coding standard. otherwise, this patch looks very good. i'll want to do a more thorough review once it is a bit more readable ;-) thx for working on this btw!
I took the previous patch and cleaned up the tabs in my editor. I did this with the intent that the reviewer might better be able to see and make comments on the patch. And perhaps help move this bug along :) Note this attachment is not from the owner of the bug, just from me to help make it more readable ....
hi. any activity on this bug? could the interested parties chime in?
i'll take ownership of this bug and try to land the patch for 0.9.9, but no guarantees ;-)
Assignee: daniel → darin
Status: NEW → ASSIGNED
Priority: -- → P5
Target Milestone: --- → mozilla0.9.9
Hi, I'm still interested in finishing this bug as well...
i'm a bit confused by the state machine impl. the STS expects Process() to return BASE_STREAM_WOULD_BLOCK if the socket should remain on the select list. it appears that BASE_STREAM_WOULD_BLOCK is never returned from Process() which would imply that Accept is blocking. is this correct/intended? have i overlooked something that still enables non-blocking Accept?
> the STS expects Process() to return BASE_STREAM_WOULD_BLOCK if the socket > should remain on the select list. Correct. > it appears that BASE_STREAM_WOULD_BLOCK is never returned from Process() which > would imply that Accept is blocking. nsServerSocket::Process returns NS_BASE_STREAM_WOULD_BLOCK during listening state (nsServerSocket::doListening). After a socket is accepted the server socket is placed back in the work queue and afterwards in the select list.
daniel: thx for clearing that up... i misread the patch :-/
ccing mstoltz for possible security issues.
sorry, i don't want to risk this one for mozilla 1.0 ... i hope everyone on this list will understand the need to limit the addition of new potentially risky features at this time.
Target Milestone: mozilla0.9.9 → mozilla1.1
I think this should definately be targeted for mozilla 1.0. From the mozilla 1.0 manifesto: * A set of promises to keep compatibility with various APIs, broadly construed (XUL 1.0 is an API), until a 2.0 or higher-numbered major release. All milestone releases and trunk development between 1.0 and 2.0 will preserve frozen interface compatibility. Mozilla 1.0 is a greenlight to hackers, corporations, and book authors to get busy building atop this stable base set of APIs. Offering a stable server socket support API for 1.0 is IMHO very important for developers to allow them a large base of users to deploy their software. If chatzilla can't get DCC support w/o this, many other projects will be stunted by the lack of such a feature. We should promise this feature set to developers.
-> moz 1.0 (i'm not making any promises that server sockets won't crash moz or otherwise cause problems... so please don't use this API unless you are willing to deal w/ being on the cutting edge.)
Target Milestone: mozilla1.1 → mozilla1.0
Comment on attachment 65927 [details] [diff] [review] readable patch - cleaned up tabs (andreww@netscape.com) r=darin
Attachment #65927 - Flags: review+
Here are some questions and comments i've got about the patch: 1. Should nsServerSocket::mService be an owning reference? right now, it's just weak. 2. Currently there is not locking around access to the member data in nsServerSocket... Do these methods require the same kind of synchronization that nsSocketTransport requires?? Especially for hte nsIRequest and Process() methods... 3. Does this work on the Mac? It's makefiles have not been updated... 4. the #ifdef XP_WIN16 in nsServerSocket is unnecessary because we no longer support that platform :-) 5. Some comments in nsIServerSocket.idl explaining what the interface and its methods do would be nice :-) 6. Since nsSTSLayer is a proper base-class (not an interface) it might be clearer to have *it* implement nsISupports and have nsServerSocket and nsSocketTransport use the NS_DECL_ISUPPORTS_INHERITIED and NS_IMPL_ISUPPORTS_INHERITED macros instead... I'll sr= the patch once i'm sure that 1 & 3 are not issues... All the others are just suggestions/questions :-) -- rick
Comment on attachment 65927 [details] [diff] [review] readable patch - cleaned up tabs (andreww@netscape.com) rick: you are right... this patch doesn't at all consider thread safety issues. mListener isn't even proxied off the socket transport thread. mState, for example, needs to be protected by a lock. it is modified both in nsServerSocket::Cancel and nsServerSocket::Process. Process runs on the socket transport thread and Cancel typically comes in from the UI thread.
Attachment #65927 - Flags: review+ → needs-work+
punt -> 1.1alpha
Target Milestone: mozilla1.0 → mozilla1.1alpha
Any chance someone could cut a branch with this in so we might start testing it out and (in general) helping out?
the patch isn't ready. there's not much point to putting it on a branch until someone has made an effort to make the patch threadsafe. as is, it is guaranteed to crash.
Target Milestone: mozilla1.1alpha → ---
mass futuring of untargeted bugs
Target Milestone: --- → Future
Sorry for the spam, I just wanted to know if this bug was left out in the cold by the recent api freezes Don
yes, intentionally. we're not ready to freeze such low-level functionality yet.
I've cleaned up Daniel's original patch, addressing some of the previous concerns over threadsafety. This patch is against the current HEAD sources, compiles cleanly, and (most importantly) it actually works.
I dont want to get my hopes up, but given the new trends in web services, file sharing, blah blah, it would be very advantageous to begin to enable some of this stuff - even if it was only some special debug pref for testing, or some branch that folks could pull and try out.
Some additional comments on my patch... 1) I did not remove any nsI* interfaces/methods. (Daniel's patch had removed nsISocketTransportService.reuseTransport() and .wakeup(), whereas I kept these.) Only public interface changes were adding createServerSocket() and createServerSocketOfType() to nsISocketTransportService, the nsIServerSocket interface, and the nsIServerSocketListener interface. 2) This patch could not realistically be a debug pref -- it adds a layer of code between the existing client sockets and the socket transport service that must be present to support server sockets. It could be a branch, though. 3) In Daniel's original patch, nsIServerSocket inherited from nsIRequest, and nsIServerSocketListener inherited from nsIRequestObserver. It made much more sense (to me) for both these classes to simply inherit from nsISupports. Is this the correct approach? 4) Any assistance in getting the Mac buildfiles updated would be appreciated. Finally... What is the chance of this patch showing up in 1.4? (1.3 probably is not realistic, as we're already out of 1.3alpha...)
I need this to implement file transfer for Jabberzilla. The sooner, the better :-)
Andrew: Thank you for working on this patch; however you should know that we are in the process of completely rewriting the socket transport. This patch will surely require significant revision once that lands.
Darin: Thank you for letting me know that. Might I suggest you include server socket support in your re-write? It's really not that complex, and especially if it's designed in from the start, there won't be (IMHO) ugly kludges such as nsSTSLayer. In the meantime, I need server sockets for my own uses, hence the updated patch. What's the timeframe for the re-write of the socket layer, and would it be appropriate to adopt this patch in the meantime?
andrew: yeah, i have in fact been keeping server socket support in the back of my mind while working on this rewrite... i agree that it should not be difficult to add support for it. the rewrite is planned for moz 1.3 beta, so i think it might be best to hold off on trying to land this patch. of course, by saying that i now better deliver on the promise to include server socket support ;-) if things go wrong with the rewrite, i'll be sure to revisit the work you have done. thanks again!
Is anyone working on updating this patch now that the socket transport stuff has landed? If not, I may try to work on it.
Stewart : please do!
/me rattles the cage.
Priority: P5 → --
Target Milestone: Future → mozilla1.6beta
Attached patch v2.0 patch (obsolete) — — Splinter Review
okay, this is a fresh patch against the current trunk. it looks very different than the other attempts because of how much the socket transport code has changed. i've changed the interface around a bit too. instead or allocating a server socket from the socket transport service, the server socket is just a component that can be instantiated using the xpcom component manager. there are two init methods that allow the server socket to be configured. i have left out any option for specifying a socket type because i just don't see it being used. i could be wrong; it is a very simple way of making the socket code more flexible, so perhaps i will add it. (PSM certainly doesn't expose SSL server socket support.) i've also provided a test program in netwerk/test/TestServ.cpp, which shows how to setup a simple HTTP-like server.
Attachment #57617 - Attachment is obsolete: true
Attachment #65927 - Attachment is obsolete: true
Attachment #109154 - Attachment is obsolete: true
Attachment #135009 - Flags: superreview?(bz-vacation)
Attachment #135009 - Flags: review?(dougt)
Comment on attachment 135009 [details] [diff] [review] v2.0 patch + nsCOMPtr<nsISocketTransportService> sts = + do_GetService(kSocketTransportServiceCID); + NS_ASSERTION(sts, "no socket transport service"); would it be better to put this into a ::Init method? + // make sure the STS sticks around + NS_ADDREF(gSocketTransportService); you could release it in the destructor
biesi: thx for looking at this patch! >+ nsCOMPtr<nsISocketTransportService> sts = >+ do_GetService(kSocketTransportServiceCID); >+ NS_ASSERTION(sts, "no socket transport service"); > >would it be better to put this into a ::Init method? no, it should never fail. both of these XPCOM components are in the same library. if GetService on the STS fails from within necko, then things are really wacked! a debug assertion should be sufficient, and i'd be happy to have it crash shortly thereafter. >+ // make sure the STS sticks around >+ NS_ADDREF(gSocketTransportService); > >you could release it in the destructor it is released in the destructor.
>it is released in the destructor. oh, indeed another comment: shouldn't base/src/nsServerSocket.h have a license header?
Attached patch v2.1 patch — — Splinter Review
biesi: yup, thanks for noticing the missing copyright header.
Attachment #135009 - Attachment is obsolete: true
Attachment #135009 - Flags: superreview?(bz-vacation)
Attachment #135009 - Flags: review?(dougt)
Attachment #135093 - Flags: superreview?(bz-vacation)
Attachment #135093 - Flags: review?(dougt)
I'm not likely to get to this until next week....
Comment on attachment 135093 [details] [diff] [review] v2.1 patch nice work... get it in so we test it out!
Attachment #135093 - Flags: review?(dougt) → review+
Comment on attachment 135093 [details] [diff] [review] v2.1 patch >Index: base/public/nsIServerSocket.idl Fix up the comments as I mentioned? In particular, make it clear that when you get a callback the client data lives on the socket transport. Also, clarify whether notifications after asyncListen and close() are sync or async (the start listening and stop listening notifications). >Index: base/src/nsServerSocket.cpp >+typedef void (nsServerSocket:: *nsServerSocketFunc)(void); Will this work even though the funcs we pass are private members and the nsServerSocketEvent struct is not a friend or anything? I don't see why this even compiles, given the following: >+ nsServerSocket *s = (nsServerSocket *) ev->owner; >+ nsServerSocketEvent *event = (nsServerSocketEvent *) ev; >+ nsServerSocketFunc func = event->func; >+ (s->*func)(); >+ EventCleanup(PLEvent *ev) >+ { >+ nsServerSocket *s = (nsServerSocket *) ev->owner; >+ NS_RELEASE(s); >+ delete ev; Probably better to |delete (nsServerSocketEvent*) ev;|, no? >+nsServerSocket::nsServerSocket() >+ if (!gSocketTransportService) >+ { >+ nsCOMPtr<nsISocketTransportService> sts = >+ do_GetService(kSocketTransportServiceCID); >+ NS_ASSERTION(sts, "no socket transport service"); This is evil.. comment that the socket transport service constructor sets gSocketTransportService? >+nsServerSocket::OnMsgListen() >+ if (!gSocketTransportService->CanAttachSocket()) >+ { ... >+ } >+ // >+ // ok, we can now attach our socket to the STS for polling >+ // >+ nsresult rv = gSocketTransportService->AttachSocket(mFD, this); Um.... Isn't this a race? If CanAttachSocket() succeeds and then we get interrupted, we could call AttachSocket when the socket transport service is not accepting sockets... Or are we guaranteed that CanAttachSocket is always called off the UI thread (and that we are on the UI thread)? >+nsServerSocket::Close() Document how this this method ends up calling OnStopListening() on the listener? Please document why this works. >Index: base/src/nsServerSocket.h >+ PRLock *mLock; Document what accesses should be protected by mLock and why? sr=bzbarsky with those issues addressed.
Attachment #135093 - Flags: superreview?(bz-vacation) → superreview+
fixed-on-trunk thanks for the review comments bz!
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
I can get an example which shows how to use a server side socket in JavaScript.
Biju: yes, i just checked in a JavaScript testcase. here's the LXR link: http://lxr.mozilla.org/mozilla/source/netwerk/test/TestServ.js NOTE: it may take an hour or so before LXR shows the file. it is there now in CVS if you want to access it that way.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: