FWD » Core Development » Base Language

Highest priority features:

CREATE SOCKET

socket object methods:
CONNECT
CONNECTED
DISCONNECT
READ
SET-READ-RESPONSE-PROCEDURE
WRITE

socket object events:
READ-RESPONSE

Other 4GL socket features:

CREATE SERVER-SOCKET statement
DELETE OBJECT for socket handles and for server socket handles

attributes (both socket and server socket objects unless otherwise noted):
BYTES-READ (non-server only)
BYTES-WRITTEN (non-server only)
HANDLE
INSTANTIATING-PROCEDURE
LOCAL-HOST (non-server only)
LOCAL-PORT (non-server only)
NAME
NEXT-SIBLING
PREV-SIBLING
PRIVATE-DATA
REMOTE-HOST (non-server only)
REMOTE-PORT (non-server only)
SENSITIVE
SSL-SERVER-NAME (non-server only)
TYPE

socket handle methods:
GET-BYTES-AVAILABLE
GET-SOCKET-OPTION
SET-SOCKET-OPTION

server socket handle methods:
DISABLE-CONNECTIONS
ENABLE-CONNECTIONS
SET-CONNECT-PROCEDURE

server socker handle events:
CONNECT

Commited to bazaar an initial set of testcases #1957: Use cases for the socket and server-socket attributes and methods, and a client - server sockets functional test.
This can be taken as starting point because, there probably are more cases to cover for conversion tests - I'll think of more situations and add them.

Some feedback:

1. Is the client-socket.p missing a wait-for statement?
2. Do the client and server work reliably? In other words, do they always succeed and work as expected or do they sometimes have some timing issues?
3. I would like to see communications of more than just 1 message in each direction. And please make a server that accepts multiple connections (it stays around for a while) so that you can start multiple clients and use them against the same server. I think it can be a case where the server will read anything sent by the client and it will reverse the text and send it back. Do create a special text that can be used to tell the server to end its operations. Then the client can be interactive, allowing the user to type text and see the responses dynamically. The user can exit the client only or exit both the client and server.
4. Build a 2nd non-interactive client that can run against the same server as #3. This should just generate random data and send it, then check the result to ensure it is correct. The program should do this for some time period or for a certain number of iterations.
5. Can you write a client without the read-response procedure? If so, then create an example like that. I think it is possible and you don't use a WAIT-FOR in that case.

1. Yes
2. Will test more.
3 - 4 built the program , unfortunately it seems to block when testing, I'm working to figure out what is happening.
5. It is possible to write the program with a read response procedure and without a wait-for in this case it will just execute the rest of the code ( in that case disconnect and make the socket ineligible)

Notes from 4GL Test results

- You can get the socket handle for which you set the response procedure by using SELF inside the procedure.
- SOCKET:READ can take the following arguments: buffer, start position, no of bytes to read, mode.
For read mode there are 2 compiler variables:
READ-AVAILABLE : 1 this blocks read until at least 1 byte is read
READ-EXACT-NUM : 2 this block read until the specified number of bytes is read.

Method Error Messages:

SERVER-SOCKET:ENABLE-CONNECTIONS
- When trying to use ENABLE-CONNECTIONS and there is already one active: Only one ENNABLE-CONNECTIONS can be active at any one time (9185) (actual message has any one time it's not a spelling mistake)
- When giving a port argument that is not a number: Unknown service <port_no> transport TCP. (5484)
- When giving an option that is not valid ( - S , - H , - PF) there are several possible error message: The -S parameter must be specified to connect. (5488) >for 2 params
Could not recognize argument: <arg_notvalid>. (301) + Unable to process parameters. (5509) + Invalid parameter string. (5510) number of params !=2 (where arg_not_valid is the first argument that is not recognized)
- When providing a parameter file that does not exist: Unable to open parameter file <param_name>, errno 2. (1247). + Invalid parameter string. (5510)

SOCKET:CONNECT
- When trying to connect to host / port where there is no server listening: Connection failure for host <host_name> port <port_no> transport TCP (9407)
- When providing wrong arguments Could not recognize argument: <arg_notvalid>. (301) + Unable to process parameters. (5509) + Invalid parameter string. (5510) where arg_not_valid is the first argument that is not recognized - this behavior doesn't seem to depend on number of args.
- When providing a parameter file that does not exist: Unable to open parameter file <param_name>, errno 2. (1247). + Invalid parameter string. (5510)

SOCKET:SET-READ-RESPONSE-PROCEDURE
- When giving non-existent procedure name: Procedure <compiled_source_file_path> has no entry point for <proc_name>. (6456)

SERVER-SOCKET:SET-CONNECT-PROCEDURE
- When giving non-existent procedure name: Procedure <compiled_source_file_path> has no entry point for <proc_name>. (6456)

5. It is possible to write the program with a read response procedure and without a wait-for in this case it will just execute the rest of the code ( in that case disconnect and make the socket ineligible)

I am suggesting that you write some example programs that have NO read-response callbacks at all. This is documented as the "Procedural model" in the Sockets section of the "Programming Interfaces" Progress book (dvpin.pdf). I don't want all of the examples using the Event model approach.

It seems there is a problem when reading in the connect procedure.
The problem seems to be with the buffer variable (even if SET-SIZE should reinitialize it)
for example if we have in the connectProcedure

PROCEDURE connProc.

  ...
   SET-SIZE(serverReadBuffer) = 64.

   /*Socket recieved as parameter*/

   DEFINE INPUT PARAMETER hSocket AS HANDLE. 

   /* Wait for response from client */

   WAIT-FOR READ-RESPONSE OF hSocket.

   MESSAGE hSocket:GET-BYTES-AVAILABLE().

   hsocket:READ(serverReadBuffer,1,hsocket:GET-BYTES-AVAILABLE(),READ-AVAILABLE).

   DEFINE VARIABLE clientMessage AS CHAR.

   clientMessage = GET-STRING(serverReadBuffer,1).
   MESSAGE clientMessage.
   ...
END.

If we send from the client socket the first time text "termite" we will get read 7 bytes available and the result text will be "termite".
If we send after that from the client the text "ana" we get 3 bytes available and the result text will be "anamite".

This seems pretty weird behavior since setSize should re-instantiate the buffer (atleast in p2j it's implemented this way ) and I've made sure to create a new socket in the client (to be sure there is no leftover from re-using the same socket).

server code

DEFINE VARIABLE serverSocket AS HANDLE.

DEFINE VARIABLE aOk AS LOGICAL.

DEFINE VARIABLE serverWriteBuffer AS MEMPTR.

DEFINE VARIABLE serverReadBuffer AS MEMPTR.

DEFINE VARIABLE serverMessage AS CHAR.

MESSAGE "We are in the socket server".

CREATE SERVER-SOCKET serverSocket.

/* Marshal data to write */

MESSAGE "Start event handling".

serverSocket:SET-CONNECT-PROCEDURE( "connProc").

aOk = serverSocket:ENABLE-CONNECTIONS( "-S 3363").

MESSAGE "Enabled connections:" aOk.

IF NOT aOk THEN

RETURN.

DISPLAY "Press q to exit server".

wait-for 'q' of current-window.

serverSocket:DISABLE-CONNECTIONS().

DELETE OBJECT serverSocket.

MESSAGE "Finished".

/* Connection procedure for server socket */

PROCEDURE connProc.

   SET-SIZE(serverWriteBuffer) = 64.

   SET-SIZE(serverReadBuffer) = 64.

   /*Socket recieved as parameter*/

   DEFINE INPUT PARAMETER hSocket AS HANDLE. 

   /* Wait for response from client */

   WAIT-FOR READ-RESPONSE OF hSocket.

   MESSAGE hSocket:GET-BYTES-AVAILABLE().

   hsocket:READ(serverReadBuffer,1,hsocket:GET-BYTES-AVAILABLE(),READ-AVAILABLE).

   DEFINE VARIABLE clientMessage AS CHAR.

   clientMessage = GET-STRING(serverReadBuffer,1).

   MESSAGE "received" clientMessage.

   IF UPPER(clientMessage) = "EXIT" THEN

       QUIT.

   DEFINE VARIABLE chArr   AS CHARACTER.

   DEFINE VAR I AS INT NO-UNDO.

   DEFINE VAR lngth AS INT.

   chArr = "".

   lngth = LENGTH (clientMessage).

   DO i = lngth TO 1 BY -1:

     chArr = chArr + SUBSTRING(clientMessage,i,1).

   END.

   serverMessage = chArr.

   MESSAGE serverMessage.

   PUT-STRING(serverWriteBuffer,1) = serverMessage.

   hSocket:WRITE (serverWriteBuffer, 1, LENGTH(serverMessage)).

   MESSAGE hSocket:BYTES-WRITTEN "bytes written".

END.

client code

DEFINE VARIABLE clientSocket AS HANDLE.

DEFINE VARIABLE acknowledge AS LOGICAL.

DEFINE VARIABLE clientWriteBuffer AS MEMPTR.

DEFINE VARIABLE cString AS CHARACTER.

DEFINE VARIABLE clientMessage AS CHARACTER FORMAT "X(25)" LABEL "Client Message".

/* Do READ-RESPONSE event handling */

MESSAGE "Start event handling".

REPEAT:

    CREATE SOCKET clientSocket.

    clientSocket:SET-READ-RESPONSE-PROCEDURE( "readProc").

    SET clientMessage WITH FRAME sock-frame.

    clientSocket:CONNECT ("-H localhost -S 3363").

    SET-SIZE(clientWriteBuffer) = 64.

    PUT-STRING(clientWriteBuffer,1) = clientMessage. 

    MESSAGE GET-STRING(clientWriteBuffer,1) clientMessage.

    clientSocket:WRITE (clientWriteBuffer,1,LENGTH(clientMessage)).

    WAIT-FOR READ-RESPONSE OF clientSocket.

    clientSocket:DISCONNECT().

    DELETE OBJECT clientSocket. 

END.

MESSAGE "Freeing up resources".

MESSAGE "Finished".

/* Read procedure for socket */

PROCEDURE readProc.

   DEFINE VARIABLE clientReadBuffer AS MEMPTR.

   MESSAGE "We are in READ-RESPONSE event procedure, readProc".

   SET-SIZE(clientReadBuffer) = 64.

   clientSocket:READ(clientReadBuffer,1,clientSocket:GET-BYTES-AVAILABLE()).

   /*In the socket procedure SELF will point to the handle of the socket for

   which this procedure is set */

   MESSAGE SELF:BYTES-READ "bytes read".

   /*Unmarshal data*/

   cString = GET-STRING(clientReadBuffer,1).

   DISPLAY cString FORMAT "X(25)" LABEL "Server reverse reply" WITH FRAME sock-frame TITLE "Client".

END PROCEDURE.

The solution for this would be to use get-bytes-available and use it in get-string, like:

   WAIT-FOR READ-RESPONSE OF hSocket.

   DEFINE VAR j AS INT.

   j = hSocket:GET-BYTES-AVAILABLE().

   hsocket:READ(serverReadBuffer,1,j,READ-AVAILABLE).

   DEFINE VARIABLE clientMessage AS CHAR.

   clientMessage = GET-STRING(serverReadBuffer,1,j).

My guess: Progress implements MEMPTRs instances literally as an internal pointer to some memory. This probably means that they allocate a block of a given size on the C runtime heap. I suspect that the allocation/deallocation/re-allocation of the same size probably can point to the same memory location. This is most likely an artifact of how the C runtime works (whichever one they are using) but it is possibly an artifact of some internal caching in the 4GL runtime. Either way, you have definitely found that the pointer is accessing the same memory AND that memory is not initialized (cleared). The most common form of C-based memory allocation routine, malloc(), does NOT clear the memory it allocates. Sometimes the memory may be clear the first time it is allocated, but the subsequent times it will have whatever you left there. It is also common for C runtimes to segment the heap into sections from which they allocate the similar sized blocks (e.g. all 64-byte blocks come from heap segment 4). Another approach is to cache pointers to recently de-allocated blocks and reuse these for subsequent requests. Both of these approaches allow some optimization in allocation time because the metadata about the block is already setup... This can result in an apparent "affinity" where repeated allocations point to the same memory.

P2J uses the Java heap and it works VERY differently. I am hopeful that 4GL code would not rely upon this behavior and it is certainly not defined as something that MEMPTR will provide. So, we will ignore it for now, unless we find a real application that depends upon it.

Tried to implement a procedural model according to hints given by documentation using CONNECTED and GET-BYTES-AVAILABLE.

However it is unclear if when we don't use events if we can still use response procedures, they don't seem to execute without events and documentation does not specify anything about them in procedural model so I'd say they don't work with procedural model.

If we can't use the response procedure for server-socket how will we get the parameter socket from connect procedure which allows to communicate with the client?(Found no example how this can be done in procedural model).

My conclusion would be the procedural model can only be implemented on the client, the server must be event driven

Submited testcases to bazaar.

However it is unclear if when we don't use events if we can still use response procedures, they don't seem to execute without events and documentation does not specify anything about them in procedural model so I'd say they don't work with procedural model.

Response procedures are called when an event occurs. When using the procedural model, there are no events and thus the response procedures don't get called. But you don't need the read-response in the procedural model. For the non-event samples, remove the read-response proc from the code and use fully procedural client code like in their examples.

If we can't use the response procedure for server-socket how will we get the parameter socket from connect procedure which allows to communicate with the client?(Found no example how this can be done in procedural model).

The server-socket is different. The connect processing can only be done using the connect event.

My conclusion would be the procedural model can only be implemented on the client, the server must be event driven

Yes.

In your opinion, are you done with this work?

I looked at the new testcases. Here is some feedback:

1. Please do not use spaces in filenames. You can use bzr move to resolve this.

2. All of your cases where you use loops (in client code), the CONNECT is part of the loop. That should be a rare case. It is fine to have 1 example like that, but to have all of them like that misses the point. My previous request for examples that transfer more data at one time is meant to be a loop that reads/writes in bulk on a single connection for the whole loop.

Corrected the name that used underscore and added 3 files with the test case described at points 3-4 using read and write in a single connection

1. Please do not use spaces in filenames.

...

Corrected the name that used underscore

My concern was with the "client- socket-multiple-conn.p" which has a space in it.

Another question: why doesn't client-procedural.p properly disconnect()/cleanup at the end? What is the behavior in that case?

Please do a version of the procedural client that has a bulk loop that uses the same connection multiple times. This is a very common case in Progress code, the event model is not as common. That is why I keep asking for more procedural examples.

Corrected the wrong name , added two new files which implement the same example from point 3 but with procedural mode.
From the documentation and coding feel I thought procedural mode for sockets is barely used compared to event driven so I focused more on event-driven.

Please remove this:

WAIT-FOR READ-RESPONSE OF clientSocket.

from all procedural mode examples.

The whole idea of procedural is to NOT use events. WAIT-FOR READ-RESPONSE is using events. Instead, just use a PAUSE 1 NO-MESSAGE inside your DO WHILE clientSocket:GET-BYTES-AVAILABLE < 1 loop to "sleep" before checking again.

I am still wondering why there is no cleanup code at the bottom of the procedural client code.

Updated the bazaar, below the wait-for read-response it was the alternate procedural equivalent so not sure why I put it there (probably got to used to event model).
Also added cleanup code (for tests it didn't seem necessary to cleanup since it seems to be done automatically when program exits).

Please work on the conversion support for sockets. I expect that you will take my recent feedback on the memptr/raw work into account when writing the sockets code. I expect the same kinds of design decisions as I requested in that task.

Create a class called com/goldencode/p2j/util/Socket for the main socket interfaces. The "server-socket" stuff should be in a class called com/goldencode/p2j/util/SocketListener. Factory methods (statics) in those classes should return a WrappedResource that can be assigned into a handle. All of the attributes and methods would then be implemented as instance methods in the respective classes. Constantin can help you with understanding the handle approach.

Creating the interface name Socket will cause ambiguities in existing code, is CommonSocket ok as a name replacement?

When I said "Create a class called com/goldencode/p2j/util/Socket for the main socket interfaces.", I was not referring to the idea of a Java "interface". I was using interfaces in the sense of an "API". I want the implementation of our sockets support to be in a p2j.util.Socket class since this name is most descriptive. Yes, we need an interface for the handle support. That interface can be CommonSocket. But the real resource should be in a Socket class. The only ambiguity is in the contained J2SE Socket instances. That is something that should be hidden and won't exist in too many places so I hope it is OK.

Conversion tests finally passed (I had some problems with ant and antlr failing and it took some time to figure out what was happening and fix it, also some differences because of majic code that was not up to date , but other than that conversion is ok). Comment for other task

While converting the previously created tests for socket attributes and methods, I encountered the following problem:
The NAME attribute is tested for read and write (as in documentation is passed as Access: Read/Write and 4GL tests determined this is possible for socket) but in CommonHandleChain the method for setting the name does not exist even if the conversion points to the method setName().
Solution 1:
We add the setName method in the CommonHandleChain where getName is also located and add implementation for all the handles that use this interface - the getName implementation will probably have to be changed too.
Solution 2:
If setter for name is not needed in the existent handle implementation we can only apply it to sockets:

We can add a new interface like CommonSetNameHandle which contains the setName method signatures, change the unwrap for assigning the NAME attribute to return a CommonSetNameHandle and change the SocketImpl class to implement also this interface besides CommonSocket.

Which of the two solutions should be applied?

I agree that we need to add some support for setName() as it does seem to be writable. But it is not clear where to add this. It seems that many more handles support it than would be included in CommonHandleChain. For example, buffers and buffer-fields support it too.

On the other hand, there are definitely some handles listed in the reference which supposedly do not support it.

Please do the following:

1. Make a list of the handles which are supposed to have this attribute and a list of those that don't support it. See the "Handle Reference" section of the 4GL Reference. Post your finding here.

2. Do a quick test of some of these handles from both lists: do the ones that have it documented actually work as read/write? And do the ones that have it missing, really not support it? The 4GL documentation can't really be trusted as you have probably already seen. Post your results here.

3. Review the results with Constantin and get his advice on where this is best located.

Besides the name attribute there is also an attribute called "INSTANTIATING-PROCEDURE" this appears in both SOCKET and SERVER-SOCKET but also in many more handles (but not implemented yet), so this should probably also be put into a more general available place for other handles to use.

Yes, do the same analysis on that one too. Please send an email to the entire team about INSTANTIATING-PROCEDURE to make sure no one else is working on it.

After testing for the possible handles that can have the argument name the following results:

Handles which supports Name attribute Read/Write:
browse, button, combo-boc , control-frame, dialog-box, editor ,field-group ,fill-in, frame, image, literal <-> text (literal can be set through SIDE-LABEL-HANDLE to a text handle so if text is writable so will literal), menu , menu-item, radio-set, rectangle, selection-list, server, server-socket, socket , slider, sub-menu ,toggle-box, window.

Handles which support Name attribute Read
procedure , buffer-object handle, Data-relation object handle, Data-source object handle, ProDataSet, query, soap-header, soap-header-entryref, source-procedure, stream ,target-procedure, temp-table (for this actually it says unable to set attribute for a static object) , this-procedure, x-document, x-noderef.

Could not get a functioning test case: asynchronous request handle (my guess is it doesn't support write for name attribute)

These also support INSTANTIATING-PROCEDURE. I'm not sure if there is a handle that does not support this.

The INSTANTIATING-PROCEDURE doesn't exist in the list of attributes to support for customer project task #1668 so it doesn't have to be implemented (implementing it might also require many changes ).
For the Name attribute we can create an interface WriteableName and add it to other handles that support it as this is implemented for them

I realized at runtime the same kind of problem happens on the SENSITIVE attribute which is already implemented inside the CommonWidget handler and is also a valid socket attribute even if it means different thing for widgets (able to recieve focus) and sockets ( able to recieve events).
The same solution would apply - move the isSensitive() and setSensitive() into another interface (Sensitive) which can be reused by widgets and sockets.

Remaining Issues:
Event support is implemented at the moment as widgets origin only, I'll have to think a bit more on how to extend this to sockets without affecting too much.
Support for SELF to be recognized from inside the response-procedure as the widget for which set the responseProcedure might also be tricky, does the customer code use self to get the socket handle inside response procedure?

def var h as handle.
h = current-window.
wait-for close of h.

1. determine the list events used by WAIT-FOR which targets a handle variable in the current project, to see how they are used.
2. for UI events, we will need to have something like eventList.addEvent(<event-name>, handle.unwrapWidget(h)) (i.e. emit the handle.unwrap API based on the event used).
3. for socket events like wait-for read-response of h, we will need to emit something like handle.unwrapSocket(h).
4. the big problem is if we have something like wait-for read-response of h or close of current-window (i.e. the wait-for loop is listening for UI and non-UI events). If there are such cases in current project, we will need to listen for both socket and UI events, and as understand the server-socket opens a port on the server machine, and not on the client machine.
5. check if there are ON statements with non-UI events, as in 4GL a statement like ON CONNECT OF h MESSAGE "connected" does compile (note that Costin didn't find a way to actually execute the trigger)
6. for ON statements, it is possible to have something like "ON <event> of <handle>", as the following code compiles and executes properly:
   

   def var h as handle.
   def var ch as char.
   form ch with frame f1.
   h = frame f1:handle.
   on GO of h message "in trigger".
   
   enable all with frame f1.
   wait-for close of current window.
   

   
   If there are such cases in the project, we will again need to emit handle.unwrap methods to get the resource based on the target event.

The alternative for handle.unwrap in the WAIT-FOR ... OF handle and ON ... OF handle cases would be to add EventList.addEvent(<event-name>, <handle>) APIs and let the runtime do the work (i.e. validate handle, unwrap the resource based on the event name, etc).

Another issue found by Costin is that SELF used in the context of a procedure (registered with a socket) and executed part of of the SET-CONNECT-PROCDEURE or SET-READ-RESPONSE-PROCEDURE will always act as a handle for the socket which has called this procedure. Thus, we will need to disambiguate between SELF used in a trigger and SELF used in such procedures.

In documentation for the SELF system handle, it states that it can be used in the event procedure of an async remote requests or an ActiveX control - will we need to implement these cases too ?

In regard to INSTANTIATING-PROCEDURE, working on that is not urgent right now.

SELF is used in the server project for SAX XML node processing callbacks accessing PRIVATE-DATA, LOCATOR-LINE-NUMBER, LOCATOR-COLUMN-NUMBER attributes. There are a couple of UI references too, but I think those may be gone in the latest codebase (sent last week). I haven't gotten reports for that yet. In the GUI project SELF will be used heavily for UI purposes, but I expect there to be a range of non-UI use cases. We should plan for full SELF support. All triggers and event procedures (named events?, socket callbacks, SAX callbacks...) probably need to have an "event target" handle set that matches the proper type. Then we will need to detect that resource type and emit the proper unwrapping inside that callback.

add EventList.addEvent(<event-name>, <handle>) APIs and let the runtime do the work (i.e. validate handle, unwrap the resource based on the event name, etc).

I prefer this approach.

Costin: when do you anticipate having a first pass for review?

I will try to get it done tomorrow if SELF support is not needed for sockets at the moment.

Yes, defer the SELF support for now.

Feedback:

1. The socket_events function in common-progress.rules seems to have the following issues:

a. It assumes that all events are socket events by default. This seems dangerous since any algorithm failure will lead to an incorrect result. In fact, I believe that the algorithm is flawed and will cause problems already.

b. The code assumes that there can never be a sockets event in the same wait-for as a non-sockets event. Is that correct? That assumption is a consequence of the design problem noted in 1a above. The way I read this code, the first event found that isn't a sockets event will abort further searching. But what is there is a non-sockets event first and then later in the wait-for there is a sockets event? Then this would give the wrong results. The proper way is to assume there is no sockets event and keep testing until one is found or there are no more events to check.

c. The event text should be compared case-insensitively, right?

2. The ui_statements.rules wait-for processing also assumes that the events are sockets-only and no mixed. This one is probably OK for now, but you should put a TODO comment in there that highlights the problem. The problems in #1 above still need to be fixed.

3. Remove the logging code printfln("READ-EXACT-NM") from literals.rules.

4. The connect(), connected() and disconnect() methods should not be in the Socket interface. We need an interface that can be shared with Server handle objects (appserver), Web Services handle objects and sockets. That common interface will be implemented by all 3 resource types and will be the hwrap for those methods in methods_attributes.rules.

5. "SetName" as an hwrap in methods_attributes.rules should be "WritableName".

6. SET-READ-RESPONSE comes alphabetically before SET-SOCKET-OPTION.

7. I'm not sure why there are changes in widget_references.rules and frame_scoping.rules. I definitely don't want sockets handles to be processed in either place. Please provide details of what that is intended to do. If we have to have other bypass code there to avoid incorrect results, that is fine. But if there needs to be some code emitted somewhere for sockets handles, that is not the place to do it. In fact, those changes look dangerous.

8. When you add a history entry, make sure there are 2 spaces after your initials.

Often you are doing this:

** 149 CS 20130204   ....

But it really needs to look like this:

** 149 CS  20130204   ....

The date column will then line up properly.

9. Please change the names of CommonSocketListener to SocketListener and CommonSocket to Socket.

10. Don't all the handle interfaces (which you have created) need to be added to HandleCommon? You only added Sensitive but it seems like the WritableName, SocketListener, Socket etc... need to be there too.

11. Please do not put an extra blank line at the beginning or end of a class.

12. Please get rid of the extra blank lines in the middle of the Sensitive interface.

13. We should have a superclass that can implement PRIVATE-DATA, NAME, TYPE, SENSITIVE... and other common attributes. Constantin: what do you think? I don't want every resource re-implementing these. This would not just be for SocketImple and SocketListenerImpl. This would be generic.

14. Don't use Socket and SocketListener as factory class names. We want those for the interfaces. Put both sets of factory methods into a SocketFactory class.

15. Why does the SocketEvents.waitFor() take a CommonFrame instance? Why is it in the ui package? This is messy and not what we want.

16. The Event support may need to be moved out of the ui package, so that it is more generic. But for now we can probably leave it alone.

7. I'm not sure why there are changes in widget_references.rules and frame_scoping.rules. I definitely don't want sockets handles to be processed in either place. Please provide details of what that is intended to do. If we have to have other bypass code there to avoid incorrect results, that is fine. But if there needs to be some code emitted somewhere for sockets handles, that is not the place to do it. In fact, those changes look dangerous.

These changes were necessary because statement like
wait-for read-response of h
Would convert to

EventList list0 = new EventList();
list0.addEvent("READ-RESPONSE", h.asWidget());

where h is a socket handle. This happens because previously wait-for was expected only for widgets.
In widget_references.rules it applied this rule for handles which is exactly the case we don't need -> add EventList.addEvent(<event-name>, <handle>) APIs and let the runtime do the work (i.e. validate handle, unwrap the resource based on the event name, etc).
Removing the "asWidget" part from widget_references.rule alone does not do it because it will fall under the frame_scoping rule which puts accessor annotation "asWidget" -> this annotation is put as methodText later, added to the frame_scoping rule to not include handles.

Ran the following test to check for wrong conversion results:

def var i as int.
form i with frame f1.

def var h as handle.
h = frame f1:handle.

on go of frame f1 do:
end.
wait-for close of frame f1.

wait-for close of h.

wait-for close of frame f1:handle.

wait-for close of h:current-window.

15. Why does the SocketEvents.waitFor() take a CommonFrame instance?

When we wait-for socket events from what I saw the ui also seems to be blocked so I assumed the CommonFrame parameter is needed just like in the case of LogicalTerminal.WaitFor() to accomplish this.
Is the CommonFrame parameter not needed in this case?

When we wait-for socket events from what I saw the ui also seems to be blocked so I assumed the CommonFrame parameter is needed just like in the case of LogicalTerminal.WaitFor() to accomplish this.
Is the CommonFrame parameter not needed in this case?

The UI is always blocked unless someone does an ENABLE (or equivalent like PROMPT-FOR/SET/UPDATE/CHOOSE...) and then goes into a WAIT-FOR statement. The WAIT-FOR will process events for all kinds of resources, but we have previously only supported it for UI events. It will be the subject of future work to rationalize our event processing approach to better fit the Progress idea. BUT we will be doing this in a way that is less dependent on the UI packages rather than more dependent.

At this time I cannot think of a reason why we would want a CommonFrame instance passed to the WAIT-FOR. In fact, I can't see how we would even know what the proper frame instance would be for such a call. There may not even be any frames in the procedure that is using this wait-for for sockets. So the frame instance must not be there.

13. We should have a superclass that can implement PRIVATE-DATA, NAME, TYPE, SENSITIVE... and other common attributes. Constantin: what do you think? I don't want every resource re-implementing these. This would not just be for SocketImple and SocketListenerImpl. This would be generic.

The only attribute which is common for all resources AFAIK is TYPE. Also, NAME and TYPE have different implementations for different resources (i.e. for procedures, NAME is the same as FILE-NAME, TYPE has different values depending on the resource, etc). We could let the superclass act as a storage location and have getter/setter implementation for these, but their initialization is resource-dependent. Also, note that SENSISITIVE is only common for widgets and sockets (I don't see other resources supporting this) and I don't think we will gain much if we'll refactor our widget impl just to extract SENSITIVE attribute and move it in a superclass.

What would be more helpful would be to generalize the resource chaining and the PREV-SIBLING and NEXT-SIBLING attributes. I think almost all resources for which we are currently adding conversion support need this. Beside Async request handle and Widgets, all other resource chains are session-based, so a session-based chain should be easy to add.

We at least need a common superclass for Socket and SocketListener so that the code for these attributes (e.g. NAME...) is minimized.

Go ahead with the session based chaining changes too.

I took a look at Costin's code and I think it will be a lot easier (and it will provide faster access) if we will move the chaining at the resource - we need to add setters for NEXT-SIBLING and PREV-SIBLING (in CommonHandleChain, with stubs in all affected classes) and keep actual references for each resource. Per session (in the factory class which creates these resources) we will keep references to only the first and last resource in the chain. When a resource is deleted, it will need to update the sibling's prev/next references (and will notify the factory/manager too, to update its first/last references).

1. create a superclass which implements CommonHandleChain interface (make it abstract as isValid needs per-resource implementation). This superclass should have fields for NAME, PREV-SIBLING, NEXT-SIBLING, TYPE, unknown state. Also, it will have a c'tor which will receive as parameter the resource's type and save it. Getters will be provided for all fields, and setters for all except TYPE. Name this HandleChain.
2. create another superclass (abstract too) with implementation for the SENSITIVE attribute (which extends superclass HandleChain).

At some point, I will refactor the procedure handle implementation using this idiom, but we will have a good start for the implementation of all other resources.

OK, I like it.

Feedback:

1. methods_attributes.rules has a "stray" equal sign (=) just after prog.kw_set_s_o. I think that would fail in testing.

2. A couple of the files still have an extra blank line at the end of the class.

Generally, I like this version. Please test conversion on the testcases you previously created.

Looks good. We will get it into testing shortly.

Eugenie will be checking in an update shortly. You will have at least 2 files to merge. Please do so quickly and then upload the final version for code review. If that passes, then we will get it into testing soon.

Everything looks OK except you have regressed something in the merge with language_statements.rules. The version there is missing the SocketFactory usage. Instead it still points to Socket and SocketListener for the create methods. Please fix that, re-test that update with your testcases and make sure they compile. Then upload the fixed update for review.

Looks good. We will get this into testing soon.

Committed conversion support for socket to bazaar as rev. 10167 and sent update mail to the team.

Costin: please put details here about what you are doing with the implementation, what is completed and what is left to be done. I also want to see a list of issues and questions that you have. Finally, please post your current code (even in an intermediate form) so I can see how it is progressing.

So far I've completed the connect (including ssl) on socket part and tested it, and also added code for read and write (not tested yet)
On server-socket I've also added code for connect (including ssl keyAlias and keypassword) but it's not tested yet it it needs some more work.On Monday I'll probably get something functional (socket, server-socket) and I can add a cleaned-up update.
For questions there are some ssl parameters which I'm not completely sure how to implement (nossesionreuse on socket, nosessioncache on server-socket)

As mentioned in note 58, please document the list of things that still need to be completed, fixed or investigated. I will review the intermediate update soon.

what is left to be done

I am still waiting to see a detailed list of what is left to be implemented OR investigated.

Code Review for

1. Remember that we do NOT put individual import statements in code unless we are needing to override a conflict.

Things like this:

import java.io.BufferedReader;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.PrintWriter;

Should really be this:

import java.io.*;

2. Please cleanup your javadoc, remove excess blank lines and otherwise fix your formatting to our standards. As Eric has mentioned in the past, it is a very good idea to review your changes before you send them. This is exactly what Eric or I will be doing when we get them. We will unzip the update and compare the changes in there with the currently checked in bzr version. We do this with graphical diffing tools such as meld.

3. The implementation of SocketImpl.valid() is probably not correct.

   /**
    * Reports if this object is valid for use.  
    *
    * @return   true if we are valid (can be used).
    */
   public boolean valid()
   {
      return true;
   }

I suspect that when the socket is disconnected, this should return false. There may be other cases where this should return false (e.g. when a communication error has occurred). Please

4. The address member seems to represent the remote side of the socket. But you are using it to report the local side's hostname and port. This seems to mean that your getLocalPort() and getLocalHost() implementations are wrong.

5. Why are you using a Reader/Writer implementation? That is for character streams. A socket is inherently a byte stream. I see nothing in the Progress documentation that suggests it is dealing with characters. In fact, because you read data into a memptr, it suggests that it is really dealing with bytes.

6. Your read method is missing the proper mode processing (READ-EXACT-NUM must read in a loop and READ-AVAILABLE is a return after the first read).

7. The write() implementation is incorrect, but that may be due to your use of the PrintWriter. Real socket writing does not always write the full amount of data that the caller of write() requested.

8. Your connect() and getParams() code is hard to read because you never put blank lines in between sections of your code. For example, when starting a new block of code (like an "if" or "for"), it is often good form to put a SINGLE blank line there to visually separate it from the rest of the code.

Starting today, please upload your latest code at the end of every day.

The list of things to be done at the moment:
On SOCKET side:
- I modified the read methods because the GET-BYTES-AVAILABLE need the number of bytes and this mean we have to actually read from the socket and put the information in a buffer from which read can take, this change still needs to be tested.
- The GET/SET-SOCKET-OPTION still needs to be researched and tested for functionality.

- Research more and test on 4gl the ssl connection case and make sure it works as expected.

- The conflicts between com.goldencode.p2j.util.Socket and java.net.Socket, I solved them by using complete name of java.net.Socket
maybe there are other better solutions ( renaming Socket to SOCKET maybe?)

SERVER-SOCKET

- research test the ssl connection logic

Both:

- Add Event support for WAIT-FOR READ-RESPONSE OF and CONNECT OF

And of course test the functionality with the existing functional tests, fix any bugs

More feedback:

This code in SocketListenerImpl.startListening() is wrong:

Socket s = new SocketImpl(socket);
ControlFlowOps.invokeIn(connectProcedure, contextHandle, s);

The parameter needs to be a handle. You are passing a WrappedResource. It needs to look like this:
)

Socket s = new SocketImpl(socket);
ControlFlowOps.invokeIn(connectProcedure, contextHandle, new handle(s));

I solved them by using complete name of java.net.Socket

That is fine for now.

Code Review:

1. Messages that embed the local host's port number must be built dynamically at runtime with the correct port number.

For example:

"Connection failure for host localhost port 1898 transport TCP." 

Port 1898 is something that was specific to your test run. That port will be different every time the code is run. The result of calling getLocalPort() needs to be substituted.

2. Why is the buffer.getBytes(pos, len); at the top of the read() method?

3. BinaryData.getBytes() is not yet implemented. So your code can't possibly be tested. You will have to do an implementation to make it work.

4. In read(), this code doesn't make any sense:

         BinaryData data = new raw(b);
         buffer.assign(data.getBytes(pos, len) );

The purpose of read() is to set a very specific range of bytes in "buffer" starting at pos and going for len (or less if not all bytes were read). This does something very different: it assigns the entire contents of "buffer" to the subset of bytes read. That is not correct. You would normally call something like BinaryData.setBytes() to do this. But I also want to avoid the intermediate creation of the "data" raw instance. Just add a setBytes(byte[], int, int) method that you can use.

4. read() still isn't supporting the different reading modes.

5. This code in read() will cause an NPE:

         byte[] b = null;
         // set the number of bytes read
         bytesRead = in.read(b);

You are required to allocate the memory for the buffer b before you call read().

6. You shouldn't access BinaryData.value directly outside of BinaryData or its subclasses, it is dangerous and a bad practice. Instead, just add a byte[] getByteRange(int pos, int len) method to do the job.

7. Why are you using DataInputStream/DataOutputStream? I don't really see any benefit to using those classes since we are always reading/writing bytes. Maybe using BufferedInputStream/BufferedOutputStream is useful. But even there, I wonder if it will cause a problem to be "disconnected" from the socket directly (e.g. for available() behavior).

8. You are writing too much code without any testing. I think you need to get some simple testcases running earlier.

Code Feedback:

1. Some of the error messages are still wrong (the host is hard coded to "localhost" but it should be dynamic):

String msg = "Connection failure for host localhost port " + port + " transport TCP.";

2. In read(), instead of calling getBytesAvailable().intValue() which has to instantiate an integer wrapper just to then unwrap it, why not just create a worker method int available() which can be used for both getBytesAvailable() and for "internal" usage like read()?

3. The logic of read() is still incorrect. Consider this code:

         byte[] b = new byte[getBytesAvailable().intValue()];
         // set the number of bytes read
         bytesRead = in.read(b);

What happens in the 4GL when the number of bytes available is zero? My understanding is that no matter what mode is given it will block and wait for at least 1 byte to be read (and possibly all bytes requested).

In addition, the read() method is supposed to read up to len bytes but you are reading up to available bytes. If available > len then you will OVERWRITE data that is NOT EXPECTED by the caller. If available < len, then you will NEVER READ up to the expected possible amount. This is especially a problem if the mode is passed as READ-EXACT-NUM.

Please get this right. If you have questions, ask them.

3. Please name the newly added BinaryData.setBytes() method as setByteRange() to make it consistent with the new getByteRange() method.

4. realLenght is mis-spelled.

5. This is wrong:

   public byte[] getByteRange(int pos, int len)
   {
      int realLenght = pos + len > value.length ? value.length : pos + len;
      byte b[] = new byte[realLenght];
      for (int i = pos; i < realLenght; i++)
      {
         b[i] = value[i];
      }
      return b;
   }

The only time you ever would want to return an array b[] of length value.length is is pos 0 and len == value.length. This is the only time your 1st line of code is correct. In other words, you should almost never be allocating a new byte[] as realLength if realLength is also meant to be the end condition of the for loop.

6. This is wrong:

   public void setBytes(byte[] source, int pos, int len)
   {
      int realLenght = (pos + len) < source.length ? len : source.length - pos;
      byte[] b = new byte[realLenght];
      for (int i = pos; i < realLenght; i++)
      {
         b[i] = source[i];
      }
      value = b;
   }

realLength should not be used as both the size of the array AND as the for loop condition.

In addition to this, why are you creating an intermediate byte[] anyway? That makes no sense and is unnecessary. In fact, because you do that and then assign value = b, your algorithm wipes away all the other existing contents of the memptr/raw EXCEPT for the part that was just passed in. That is just as bad as your previous code that just called setBytes() from the socket processing code.

7. SocketListenerImpl.valid() may not be correct. Just like with the socket handle, there are probably times when the server socket resource is invalid.

Code Review:

1. getByteRange/setByteRange: have you tested these? I don't think they can work properly. For example, pos is a 1-based index, but you are not ever converting from 1-based to 0-based before using it. Please test these and get the implementation correct. It should be pretty simple to write hand-written code to test both methods.

2. This code is closer but I doubt it is correct:

            String msg = "Connection failure for host " + getLocalHost() +
                         " port " + port + " transport TCP.";

I suspect that instead of getLocalHost() it should be getRemoteHost() here. Please confirm this on the 4GL system by actually trying to connect to another system (one that doesn't even exist is fine) and look at the resulting host and port in the message.

3. The read() code is better, but it is still not correct. For example, this code in the READ_AVAILABLE section is really bad:

           while(bAvail == 0)
            {
               bAvail = available();
            }

That is just going to sit in a very tight CPU loop and is a bad practice. Please never do this.

And it is not needed anyway. You don't need to allocate a buffer of size available. Just allocate a buffer of the proper max size that the caller gave you (using len) and simply try to in.read() using the allocated array. It will block until something can be read. It will not read more than the caller wanted and it will return as soon as it has read at least 1 byte. This is the exact implementation of READ_AVAILABLE as far as I understand it. It is very simple.

In the section on READ_EXACT_NUM, you don't have to read 1 byte at a time. You can and should read into the array, using the number of read bytes to increment the "pos" and "len" that you pass into read. Otherwise, the result will be very slow.

Accepting connection for ServerSocket doesn't seem to be done async as I previously implemented in the startListening method, however it does seem to collect connect events and keep them to be executed in a WAIT-FOR, but as well as an input blocking statement:

Example: Here I replaced the WAIT-FOR CONNECT with an UPDATE variable statement and the connect procedure will get executed.

DEFINE VARIABLE serverSocket AS HANDLE.
DEFINE VARIABLE aOk AS LOGICAL.
DEFINE VARIABLE serverWriteBuffer AS MEMPTR.
DEFINE VARIABLE serverReadBuffer AS MEMPTR.
DEFINE VARIABLE serverMessage AS CHAR.
MESSAGE "We are in the socket server".

CREATE SERVER-SOCKET serverSocket.
/* Marshal data to write */
SET-SIZE(serverWriteBuffer) = 64.
SET-SIZE(serverReadBuffer) = 64.
serverMessage = "SERVER - hello".
PUT-STRING(serverWriteBuffer,1) = serverMessage.
MESSAGE "Start event handling".

serverSocket:SET-CONNECT-PROCEDURE( "connProc").
aOk = serverSocket:ENABLE-CONNECTIONS( "-S 3363").
MESSAGE "Enabled connections:" aOk.
IF NOT aOk THEN
RETURN.

message "client sends the message now...".
pause.

/* This WAIT-FOR completes after the first connection */
UPDATE aok WITH FRAME fa.
/*WAIT-FOR CONNECT OF serverSocket.*/
MESSAGE "FIRST CONNECTION RECEIVED".

serverSocket:DISABLE-CONNECTIONS().
DELETE OBJECT serverSocket.
MESSAGE "Finished".

/* Connection procedure for server socket */
PROCEDURE connProc.
   /*Socket recieved as parameter*/
   DEFINE INPUT PARAMETER hSocket AS HANDLE. 
   MESSAGE "We are in CONNECT event procedure, connProc".
   hSocket:WRITE (serverWriteBuffer, 1, LENGTH(serverMessage)).
   MESSAGE hSocket:BYTES-WRITTEN "bytes written".
   /* Wait for response from client */
   WAIT-FOR READ-RESPONSE OF hSocket.
   hsocket:READ(serverReadBuffer,1,hsocket:GET-BYTES-AVAILABLE()).
   DEFINE VARIABLE clientMessage AS CHAR.
   clientMessage = GET-STRING(serverReadBuffer,1).
   DISPLAY clientMessage FORMAT "x(64)".
END.

As a solution in the startListening() methods I started a new thread which accepts connections and then registers a new Socket event like this.

Thread t = new Thread()
      {
         public void run()
         {

            while (keepListening)
            {
               try
               {
                  java.net.Socket socket = socketListener.accept();;
                  SocketEvents.registerEvent(socket,connectProcedure, contextHandle);
               }
               catch (IOException e)
               {
                  // TODO Auto-generated catch block
                  e.printStackTrace();
               }
            }
         }
      };
      t.start();

Now the problem with the StackOverflow remains because the thread that is collecting the connection events is not registered with the SessionManager.

Currently it doesn't seem to be a way to register the thread because the SessionManager setContext methods are protected and located in another package.
What would you advise?

however it does seem to collect connect events and keep them to be executed in a WAIT-FOR, but as well as an input blocking statement:

Example: Here I replaced the WAIT-FOR CONNECT with an UPDATE variable statement and the connect procedure will get executed.

Yes, this is expected. UPDATE uses several other "primitive" operations under the covers, including WAIT-FOR. I also expect a PROCESS EVENTS statement would probably do the trick.

Now the problem with the StackOverflow remains because the thread that is collecting the connection events is not registered with the SessionManager.

Please show the stack trace here (at least the important parts).

Currently it doesn't seem to be a way to register the thread because the SessionManager setContext methods are protected and located in another package.
What would you advise?

Before we get to this question, we actually have an important design decision to make: should the sockets processing run on the server or on the client?

I am pretty sure that each appserver "agent" and each batch process can have its own server socket opened. That means that we would have to "virtualize" the processing if we want it in our P2J server. This is optimal for performance, but we need to determine if there are any features that would be missing or impossible to implement/virtualize on the server side.

Constantin: what do you think?

Here is the partial message of StackOverflow

Exception in thread "Thread-8" java.lang.StackOverflowError
    at java.util.regex.Pattern$2.isSatisfiedBy(Pattern.java:3621)
    at java.util.regex.Pattern$6.isSatisfiedBy(Pattern.java:4763)
    at java.util.regex.Pattern$6.isSatisfiedBy(Pattern.java:4763)
    at java.util.regex.Pattern$CharProperty.match(Pattern.java:3345)
    at java.util.regex.Pattern$GroupHead.match(Pattern.java:4168)
    at java.util.regex.Pattern$Branch.match(Pattern.java:4112)
    at java.util.regex.Pattern$Branch.match(Pattern.java:4112)
    at java.util.regex.Pattern$Branch.match(Pattern.java:4112)
    at java.util.regex.Pattern$BranchConn.match(Pattern.java:4078)
    at java.util.regex.Pattern$GroupTail.match(Pattern.java:4227)
    at java.util.regex.Pattern$Curly.match0(Pattern.java:3789)
    at java.util.regex.Pattern$Curly.match(Pattern.java:3744)
    at java.util.regex.Pattern$GroupHead.match(Pattern.java:4168)
    at java.util.regex.Pattern$Branch.match(Pattern.java:4114)
    at java.util.regex.Pattern$Branch.match(Pattern.java:4112)
    at java.util.regex.Pattern$BmpCharProperty.match(Pattern.java:3366)
    at java.util.regex.Pattern$Start.match(Pattern.java:3055)
    at java.util.regex.Matcher.search(Matcher.java:1105)
    at java.util.regex.Matcher.find(Matcher.java:561)
    at java.util.Formatter.parse(Formatter.java:2461)
    at java.util.Formatter.format(Formatter.java:2414)
    at java.util.Formatter.format(Formatter.java:2367)
    at java.lang.String.format(String.java:2769)
    at com.goldencode.p2j.util.LogHelper.describeContext(LogHelper.java:490)
    at com.goldencode.p2j.util.LogHelper.generate(LogHelper.java:355)
    at com.goldencode.p2j.directory.DirectoryService.log(DirectoryService.java:402)
    at com.goldencode.p2j.directory.DirectoryService.log(DirectoryService.java:379)
    at com.goldencode.p2j.directory.DirectoryService.logTrace(DirectoryService.java:464)
    at com.goldencode.p2j.directory.DirectoryService.bind(DirectoryService.java:1784)
    at com.goldencode.p2j.util.Utils.getDirectoryNodeWorker(Utils.java:1859)
    at com.goldencode.p2j.util.Utils.getDirectoryNodeBoolean(Utils.java:1699)
    at com.goldencode.p2j.util.Utils.getDirectoryNodeBoolean(Utils.java:1420)
    at com.goldencode.p2j.util.TransactionManager$ContextContainer.obtain(TransactionManager.java:5537)
    at com.goldencode.p2j.util.TransactionManager.<init>(TransactionManager.java:528)
    at com.goldencode.p2j.ui.LogicalTerminal.<init>(LogicalTerminal.java:718)
    at com.goldencode.p2j.ui.LogicalTerminal.<init>(LogicalTerminal.java:687)
    at com.goldencode.p2j.ui.LogicalTerminal$1.initialValue(LogicalTerminal.java:583)
    at com.goldencode.p2j.ui.LogicalTerminal$1.initialValue(LogicalTerminal.java:1)
    at com.goldencode.p2j.security.ContextLocal.get(ContextLocal.java:170)
    at com.goldencode.p2j.ui.LogicalTerminal.locate(LogicalTerminal.java:5799)
    at com.goldencode.p2j.ui.LogicalTerminal.access$5(LogicalTerminal.java:5797)
    at com.goldencode.p2j.ui.LogicalTerminal$3.createScopeable(LogicalTerminal.java:6422)
    at com.goldencode.p2j.util.TransactionManager$ContextContainer.obtain(TransactionManager.java:5556)
    at com.goldencode.p2j.util.TransactionManager.<init>(TransactionManager.java:528)
    at com.goldencode.p2j.ui.LogicalTerminal.<init>(LogicalTerminal.java:718)
    at com.goldencode.p2j.ui.LogicalTerminal.<init>(LogicalTerminal.java:687)
    at com.goldencode.p2j.ui.LogicalTerminal$1.initialValue(LogicalTerminal.java:583)
    at com.goldencode.p2j.ui.LogicalTerminal$1.initialValue(LogicalTerminal.java:1)
    at com.goldencode.p2j.security.ContextLocal.get(ContextLocal.java:170)
    at com.goldencode.p2j.ui.LogicalTerminal.locate(LogicalTerminal.java:5799)
    at com.goldencode.p2j.ui.LogicalTerminal.access$5(LogicalTerminal.java:5797)
    at com.goldencode.p2j.ui.LogicalTerminal$3.createScopeable(LogicalTerminal.java:6422)
    at com.goldencode.p2j.util.TransactionManager$ContextContainer.obtain(TransactionManager.java:5556)
    at com.goldencode.p2j.util.TransactionManager.<init>(TransactionManager.java:528)
    at com.goldencode.p2j.ui.LogicalTerminal.<init>(LogicalTerminal.java:718)
    at com.goldencode.p2j.ui.LogicalTerminal.<init>(LogicalTerminal.java:687)
    at com.goldencode.p2j.ui.LogicalTerminal$1.initialValue(LogicalTerminal.java:583)
    at com.goldencode.p2j.ui.LogicalTerminal$1.initialValue(LogicalTerminal.java:1)
    at com.goldencode.p2j.security.ContextLocal.get(ContextLocal.java:170)
...

The problem seems to happen in ProcedureManager.thisProcedure() method as a result of the ControlFlowOps.invoqueIn() call

I am pretty sure that each appserver "agent" and each batch process can have its own server socket opened. That means that we would have to "virtualize" the processing if we want it in our P2J server. This is optimal for performance, but we need to determine if there are any features that would be missing or impossible to implement/virtualize on the server side.

Constantin: what do you think?

I would like to see the socket events registered on the client side, so that the event loop can pick them up and process them naturally. When processing a socket event, then client will have to dispatch a request to the server which will execute the CONNECT procedure. For the actual socket creation I too think that the client side is best, but as in 4GL the client and server are on the same machine, the same port can be used for only one socket. Also, the client will have to export socket-related APIs, so the socket is available on the server too.

But the performance improvement by hosting all the socket read/write directly in the server, is a substantial benefit.

If the only thing that the client really needs to do is the event processing, then we really should consider leaving the rest on the server.

Then again, in the GUI project or even in a traditional CHUI client scenario, the client can be (and often is) running on a different system than the server. In such a case, the sockets process HAS TO BE on the client, because the ports could be used independent of the ports on the server. AND the customer's environment may even be configured to allow the client systems to access certain resources but the servers are not configured that way (i.e. the firewall configurations may be specific to a client-system environment).

With this in mind, I think the low level sockets processing needs to be on the client side. As such it will be easy to integrate with the UI event model. The CONNECT and READ-RESPONSE events can be implemented similar to triggers OR possibly even by using the trigger infrastructure.

Only the portions of the socket processing that interact with the J2SE classes need to be on the client. Maximize what can be done on the server. When you have a proposed interface for the client/server remote connection, include it for review before you code the implementation.

Started the interface for the SERVER-SOCKET methods we need on client side and looked into the logic for SSL socket and to me it seems that even though the sockets and server sockets are opened on the client, the digital certificates are located on the server and the private and public keys are sent to the client when creating the SSL ServerSocket and also the SSL Socket will get the public key from the server.

The default key alias when creating a ssl server-socket is "default_server" which seems to argument this more.
There are also some things that indicate that more than one digital certificates might exist since you can provide a password for the alias and not use the default which opens the certificate containing default_server alias.

For SSL connection this means I'll also have to add in the client side logic for simple socket a way to retrieve the public key, and on the server-socket a way to retrieve the public and private key from the server

What is you view on this?

Feedback:

1. ClientSocket should not be in the ui package. It has nothing really to do with UI. Rename it to LowLevelSocket and put it in util/.

2. A Socket instance cannot be passed from the server to the client (or vice versa). This means that setSocket(Socket socket) cannot be in LowLevelSocket. The approach has to be done completely on the client.

the digital certificates are located on the server and the private and public keys are sent to the client when creating the SSL ServerSocket and also the SSL Socket will get the public key from the server.

3. I think this is a good initial idea.

The default key alias when creating a ssl server-socket is "default_server" which seems to argument this more.

4. The key alias names won't have anything to do with whether this is done on the client or server.

There are also some things that indicate that more than one digital certificates might exist since you can provide a password for the alias and not use the default which opens the certificate containing default_server alias.

5. It is common for keystores/truststores to contain multiple entries, which can then be found by their aliases.

For SSL connection this means I'll also have to add in the client side logic for simple socket a way to retrieve the public key, and on the server-socket a way to retrieve the public and private key from the server

I would need to see the code to really know for sure. But generally, you should plan for the server to hand the client the details it needs to do its job with SSL. You can look inside our SecurityManager code for some useful examples. I prefer for the server to send the needed data down instead of the client calling up to get the data.

Regarding certificate issue for ssl sockets and server sockets: after looking in SecurityManager getTransportSecurity it seems that the key store certificates are loaded both in client and server through this in the TransportSecuriy class, so I'm not sure if we actually need to handle loading the certificates for the SERVER-SOCKET logic unless we have a certificate another keystore which is not specified in the bootstrap (server.xml).

Also on the client side it is specified the the input for getting the configuration parameters can also be an URL, I'm curious about how this url is used and looks like.

The client side loading of certificates is only for securing the connection between a P2J client and a P2J server. That is not related to implementing the client-side 4GL sockets support. All the certificate information will have to come from the server.

In addition, we don't want the server to have to have the certificates passed via bootstrap configuration. The stuff in the bootstrap config is there to be the minimum configuration needed to get the P2J server started. Once the server can read its own directory, we would expect all the certificate data to be found there.

1. server and client sockets are opened by the P2J client
2. LowLevelSocket is an interface which defines the client-socket APIs which need to be executed at the P2J client
3. LowLevelSocketListener is an interface which defines the server-socket APIs which need to be executed at the P2J client
4. the P2J client needs to assign each open socket an unique ID. LowLevelSocket.connnect[Ssl] and LowLevelSocketListener.enable[SSL]Connections will return the socket id, if the socket was opened
5. SocketListenerImpl is the SOCKET-SERVER implementation on P2J server side:
   - field LowLevelSocketListener clientSocketListener is a network proxy for the LowLevelSocketListener implementation on the P2J client side (exported by the P2J client)
   - field long socketId is 1 by default and will be set when the ENABLE-CONNECTIONS is called (and the call was successful).
   when calling exported APIs via clientSocketListener, it must check the socketId before calling and also it must pass this id to the API
6. SocketImpl is the SOCKET implementation on P2J server side:
   - the LowLevelSocket clientSocket is a network proxy for the LowLevelSocket implementation on the P2J client side (exported by P2J client)
   - the long socketId is 1 by default and must be set when CONNECT is called
   when calling exported APIs via clientSocket, it must check the socketId before calling and also it must pass this id to the API
7. the implementation of LowLevelSocketListener.enable[SSL]Connections must register each opened socket in a map, by its computed id. When other LowLevelSocketListener APIs are called, it will use this map to determine which socket to use (and maybe other info too)
8. on the P2J client, when LowLevelSocketListener.enable[SSL]Connections is called, it will create a new thread which will listen for connections on that server socket. When a request is accepted, it must:
   - register the accepted socket in the socket map (and assign an ID)
   - add a new CONNECT event to the event queue (not sure yet if we is OK to use separate queue)
   - when adding an event to the queue, check if the queue is not full (based on the qsize value); else, refuse the connection. As a P2J client can open multiple server socket, the number of incoming requests must be kept by each server socket.
9. on the P2J client, when processing an incoming request, we will need another interface (this time exported by the P2J server). Lets call this SocketEventsHandler. This will have two methods:
   - connect(int serverSocketId, int socketId); this will be called by the P2J client when processing a CONNECT event
   - readResponse(int socketId); this will be called by the P2J client when processing a READ-RESPONSE event
   Also, on an incoming request, a new thread needs to be created, which will wait for data to be available on the socket's input stream. When data is available, a READ-RESPONSE event will be generated for this socket. Note that we can't read the data here.
10. on the P2J server, when SET-CONNECT-PROCEDURE is called for a SERVER-SOCKET, it will save at the SocketListenerImpl class, the procedure name and the external procedure handle. When the P2J server needs to execute the SocketEventsHandler.connect implementation, it will:
    - find the SocketListenerImpl associated with the serverSocketId (will need to keep a server-socket registry on the P2J server too, in a context-local map at SocketListenerImpl
    - get the info about which procedure needs to be called (name and handle)
    - build a new handle, with a SocketImpl instance as resource, which will use the passed socketId
    - invoke the procedure
    - when the procedure uses the passed handle to send/receive data or wait for READ-RESPONSE events, it will pass the socketId to the LowLevelSocketAPI

I'm out of the office for ~3 hours, I'll provide the details about SSL authentication and socket event processing when I get back.

About qsize parameter for ENABLE-CONNECTIONS (related to point 8 at note 91). We need to determine where we put the limitation. The docs for this state that:

Optional. The length of the pending-connection
queue—that is, the maximum number of connection
requests you want the server to queue while it
processes the connections already accepted.
If the queue is full when a connection request is
received, it is refused.
The default length of the queue depends on the
platform.

And also, it looks like if not set, a default value is used:

Note: On some platforms, the value you supply for
backlog is modified by addition, subtraction,
multiplication, division, or some combination of these,
and it is this modified value that becomes the
maximum length of the queue. For more information,
see the documentation for your platform.

The approach and the questions both look good.

But, other info we need to determine is this: if the qsize is 2 and on the first connected client, the CONNECT procedure is blocked i.e. waiting for user input, what happens when another client wants to establish a connection? Does the connection attempt timeout at some point? It can't be refused, as the incoming connection queue is not full. If it timeouts, what is the timeout and error message?

I've tested this on 4GL by running the a program which creates 900 sockets sequentially and connects them to a server which accepts connections in a loop with a qsize set to 1.

From the test I saw that an error didn't happen straight away which might mean 2 things there was a "lag" until the first connection from the second terminal was made or as soon as connection is accepted() the socket is removed from queue and it didn't reach a size greater than 1 straight away because we didn't have two sockets connecting at the same exact time.

Eventually the error

Connection failure for "host_name" port "port_number" transport TCP. (9407)

None of the widgets used in the WAIT-FOR statement are in a state (such as SENSITIVE) such that the specified event can occur.WAIT-FOR terminated.(4123)

Also I got an error regarding WAIT-FOR statement inside the server on the socket it accepts in the connect procedure:

Cannot execute WAIT-FOR statement when a invalid widget handle is encountered.(4012)

Invalid handle. Not initialized or points to a deleted object. (3135)

Costin: please look through the OE 10.1B docs and see if you can find what this default value is (search for "qsize" or other relevant text you can think of).

I haven't managed to find any info about a default value for qsize apart the fact that it is OS dependent.

In java we can initialize a ServerSocket using the port or the port and qsize equivalent, if not provided it might also use a default qsize which might be equivalent to the 4GL one if it's OS-dependent.

The code I used for testing:
client

DEFINE VARIABLE clientSocket AS HANDLE.
DEFINE VARIABLE acknowledge AS LOGICAL.
DEFINE VARIABLE clientWriteBuffer AS MEMPTR.
DEFINE VARIABLE cString AS CHARACTER.
DEFINE VARIABLE clientMessage AS CHARACTER.
DEFINE VARIABLE i AS INTEGER.
i=0.
CREATE SOCKET clientSocket.
SET-SIZE(clientWriteBuffer) = 64.
/* Do READ-RESPONSE event handling */
MESSAGE "Start event handling".
clientSocket:SET-READ-RESPONSE-PROCEDURE( "readProc"). 
clientMessage = "REVRES".
DO WHILE i < 900: 
    clientSocket:CONNECT ("-H localhost -S 3363").
    PUT-STRING(clientWriteBuffer,1) = clientMessage. 
    clientSocket:WRITE (clientWriteBuffer,1,LENGTH(clientMessage)).
    i = i + 1.
    WAIT-FOR READ-RESPONSE OF clientSocket.
    clientSocket:DISCONNECT().
END.

MESSAGE "Freeing up resources".

DELETE OBJECT clientSocket.
MESSAGE "Finished".

/* Read procedure for socket */
PROCEDURE readProc.
   DEFINE VARIABLE clientReadBuffer AS MEMPTR.
   SET-SIZE(clientReadBuffer) = 64.
   DEFINE VAR j AS INTEGER.
   j = clientSocket:GET-BYTES-AVAILABLE().
   clientSocket:READ(clientReadBuffer,1,j).

   /*In the socket procedure SELF will point to the handle of the socket for
   which this procedure is set */
   MESSAGE clientSocket:BYTES-READ "bytes read".
   /*Unmarshal data*/
   cString = GET-STRING(clientReadBuffer,1,j).
   IF (cstring = "SERVER") THEN 
       MESSAGE "Read ok".
   ELSE DO:
       MESSAGE "Error receiving message".
   END.
END PROCEDURE.

server

DEFINE VARIABLE serverSocket AS HANDLE.
DEFINE VARIABLE aOk AS LOGICAL.
DEFINE VARIABLE serverWriteBuffer AS MEMPTR.
DEFINE VARIABLE serverReadBuffer AS MEMPTR.
DEFINE VARIABLE serverMessage AS CHAR.
MESSAGE "We are in the socket server".

CREATE SERVER-SOCKET serverSocket.
/* Marshal data to write */

MESSAGE "Start event handling".

serverSocket:SET-CONNECT-PROCEDURE( "connProc").
aOk = serverSocket:ENABLE-CONNECTIONS("-S 3363 -qsize 1").
MESSAGE "Enabled connections:" aOk.
IF NOT aOk THEN
RETURN.
DISPLAY "Press q to exit server".
wait-for 'q' of current-window.

serverSocket:DISABLE-CONNECTIONS().
DELETE OBJECT serverSocket.
MESSAGE "Finished".

/* Connection procedure for server socket */
PROCEDURE connProc.
   SET-SIZE(serverWriteBuffer) = 64.
   SET-SIZE(serverReadBuffer) = 64.
   /*Socket recieved as parameter*/

   DEFINE INPUT PARAMETER hSocket AS HANDLE. 
   /* Wait for response from client */
   WAIT-FOR READ-RESPONSE OF hSocket.
   DEFINE VAR j AS INT.
   j = hSocket:GET-BYTES-AVAILABLE().
   hsocket:READ(serverReadBuffer,1,j,READ-AVAILABLE).
   DEFINE VARIABLE clientMessage AS CHAR.
   clientMessage = GET-STRING(serverReadBuffer,1,j).
   MESSAGE "received" clientMessage.
   IF UPPER(clientMessage) = "EXIT" THEN
       QUIT.
   DEFINE VARIABLE chArr   AS CHARACTER.
   DEFINE VAR i AS INT NO-UNDO.

   DEFINE VAR lngth AS INT.
   chArr = "".
   lngth = LENGTH (clientMessage).
   DO i = lngth TO 1 BY -1:
     chArr = chArr + SUBSTRING(clientMessage,i,1).
   END.
   serverMessage = chArr.
   MESSAGE serverMessage.
   PUT-STRING(serverWriteBuffer,1) = serverMessage.
   hSocket:WRITE (serverWriteBuffer, 1, LENGTH(serverMessage)).
   MESSAGE hSocket:BYTES-WRITTEN "bytes written".

END.

Costin: please test by adding a PAUSE in the server's code, right at the top of the connProc procedure. On the first connection, the server block until key press. After this, start another instance of the client program in a different terminal and see how it reacts. Test with and without your "DO WHILE i < 900:" loop from the client.

About ENABLE-CONNECTIONS when the socket needs to be in SSL mode; as the private keys should be on the P2J server and the P2J client which will create the SSL socket needs access to them, we need to provide them to the P2J client, when LowLevelSocketListener.enableSSLConnections is called. We can read the store file into a byte array and add these parameters to the enableSSLConnections API:
1. byte[] keyStore - the store file as a byte array
2. char[] keyStorePassword
3. char[] keyEntryPassword
On P2J client, the following code can be used to create the SSL socket:

// create SSL context
SSLContext ctx = SSLContext.getInstance("TLS");
// initialize the KeyStore and read the keys from it
KeyManagerFactory kmf = KeyManagerFactory.getInstance("SunX509");
KeyStore keyStore = KeyStore.getInstance("JKS");
InputStream keyIn = new ByteArrayInputStream(store);
keyStore.load(keyIn, keyStorePassword);
kmf.init(keyStore, keyEntryPassword);
KeyManager[] km = kmf.getKeyManagers();
// get the trust store - I think the store file should be on the client
// TrustManager[] tm = getTrustManagers();

// initialize SSL context
ctx.init(km, tm, null);
// create socket
SSLServerSocketFactory factory = ctx.getServerSocketFactory();
secure = (SSLServerSocket) factory.createServerSocket(port);
secure.setWantClientAuth(true);

In the docs, when creating a SSL SERVER-SOCKET, is possible to specify a keyaliaspasswd, which I guess is the keyEntryPassword. Also, if no alias is specified, a default_server alias is used; and if also no password is specified, then a default password is used.

Note that if the client is in insecure mode, the keyStore and passwords will be sent "clear text" from the P2J server to the P2J client - should this be a concern?

Also, when a client needs to connect to a SSL SERVER-SOCKET, should it validate the received certificate with a trust-store? If yes, the trust store should be placed on the client.

Note that if the client is in insecure mode, the keyStore and passwords will be sent "clear text" from the P2J server to the P2J client - should this be a concern?

It should be documented clearly, but otherwise we won't be addressing this right now. Using insecure sockets is a choice that can expose this and much more. That is why we recommend exclusively using secure sockets. But customers can choose.

when a client needs to connect to a SSL SERVER-SOCKET, should it validate the received certificate with a trust-store? If yes, the trust store should be placed on the client.

If the 4GL does this, then we will need to do so too.

Tested in 4GL (the example with Pause) and for qsize and it seems that after the queue is full connections are rejected.
Tried the same thing with Java instantiating a ServerSocket with the given backlog - which has the following javadoc:

The maximum queue length for incoming connection indications (a request to connect) is set to the backlog parameter. 
If a connection indication arrives when the queue is full, the connection is refused.

The last part doesn't seem to happen exactly like described.

To test this I created 100 threads in which a java.net.Socket is created connects to the server, writes something and reads something back.

On the server in the thread that accepts connection I've put a sleep of 10 seconds before another accept is made , during which time it should put the pending connection in the queue.

The result:
Even if the queue size is set to 1, the sockets that connect from the client will show that they are successfully connected even if they should've been rejected from the queue.

None of the 100 sockets were rejected when I tested with pause before accepting anything on the server the only problem occuring when trying to read on client side -> java.net.ConnectException: Connection timed out,

A few were not connected when I tested with pause after accepting on the server - the cause :java.net.ConnectException: Connection timed out).

This means that qsize will probably need to have it's own implementation since we can't rely on the Java ServerSocket backlog.

about SSL connections:

For Milestone 7, we don't need SSL support ("-ssl" never appears in any CONNECT string input and ENABLE-CONNECTIONS is not used in the server project). Please create a separate task for the SSL work. We will defer that.

a. OpenEdge keeps the private keys in a global store. Also, all certificates are kept in a global trust store.

Does this get kept for a Progress installation (i.e. all _progres executables started from the same installation dirs get the same keystores)?

Or does this get created dynamically at runtime when the client needs it?

Or does each client have persistent and unique keystore files?

Generally, we would want to reuse our certificate infrastructure in our directory. I really don't want to keep separate keystore files in the filesystem. If the current cert support doesn't handle our needs, then we can extend it or (worst case) put in custom features that are backed by the directory.

d. Some SSL options don't look to have a counterpart in JSSE:

Document this throughly. We won't worry about these now. There may come a time when these are important and in that case we will look deeper.

I think we should modify memptr to allow it to be used on P2J Client side when reading/writing; this will allow us access to the real memory, and will not impose limitations on the size of data we can read/write.

Yes, I agree. I also would like to see this as a possibility to avoid unnecessary copying of data between the P2J client and the P2J server. Of course, the data must be put into (or gotten from) the memptr. But when moving that to/from the socket, we should not copy the memptr data to the server just so that we can then copy it back down to the client for output on a socket.

Using SocketChannel Socket.getChannel(), is not feasible, as the javadoc states that:

A socket will have a channel if, and only if, the channel itself was created via the SocketChannel.open or ServerSocketChannel.accept methods.

Considering that we need to allow a socket to connect to a non-4GL server-socket or a server-socket to receive connections from a non-4GL socket, I don't think this will work, as my understanding from the docs is that both sides need to use the SocketChannel for read/write, for this to work.

I don't think your conclusion is correct here. Yes, to get a Socket/ServerSocket instance that is enabled for NIO use (i.e. is a channel-aware implementation), you must get a SocketChannel/ServerSocketChannel first and then obtain the "contained" Socket or ServerSocket instance. But that has no bearing on what is actually read/written to the socket. As far as I know, the server or client on the other end of the socket can be anything (not just Java). There is no serialization or other proprietary data sent over the socket. I think it is OK to use the NIO stuff if it helps.

What leads you to make your conclusion?

Code Review 0916c

General: very good work. The multithreaded read part is especially hard to get one's mind around. That is not your fault.

Specific feedback:

1. We should not be using Apache Commons logging. Eric's use of it was unsanctioned and should not be extended. We were going to remove that usage... we don't want to extend it.

2. How does the 4GL handle negative values or very large values for pos or len in read() and write() methods?

3. Is it safe to use the hash code for the ID to resource mapping in handle? It seems like this could easily return back the wrong resource or have a resource overwritten because of conflicts. Is the hash code always unique for all resource types? Even if it is, someone could come along later and change the implementation without realizing that we have a dependency on it...

4. The formatting of the header in ConnectHelper is slightly off.

5. Why is the LowLevelSocketImpl stuff initialized in ClientCore instead of with the other "daemons" in ThinClient? At some point soon, we will have to move all that code out of the TC to a clinet-specific location that is not UI-related. BUT I prefer to have all that code in one place for now, so it is easy to find.

6. The change to frame_scoping.rules is_potential_widget() seems dangerous. Please explain the idea.

7. It seems like there is very similar code implemented in both the socket and server-socket classes. Is there any opportunity to make some common base classes?

8. StagedInput is writing some log entries to STDOUT instead of using real logging support.

9. Should the LowLevelSocket.read/write methods take a BinaryData or should they take a memptr? Somehow I thought that raw could not be used there.

- for read, I chose an arbitrary chunk size of 512 bytes (although I think a larger size would work too).

Please increase this size to at least 4096. In our memory-mapped FileStream implementation, I set the upper limit at 256K and we got good performance results without ever having too much of an impact on the size of the heap. With sizes lower than the common Ethernet frame size (1500 bytes) I am concerned with the mismatch. There are other limits too like the common Jumbo Ethernet frame size (9000 bytes) or the maximum TCP packet size (64KB).

Anyway, what I'm missing is a native method which copies a byte buffer to a memory position (a setBytes(byte[] buffer, pos), the counterpart for byte[] getBytes(pos, len)). Now I'm writing it to the native memory "byte-by-byte", which I think might be slow.

Please do add it. The implementation should be straightforward. Let me know if you hit any issues or have questions.

I too think your concerns are valid. More, I think there are a couple of other issues with it:

Document the problems in a new task. Set the target milestone to 11.

I'm OK with the changes. I assume all the socket testcases (other than SSL) are working now?

If all the socket testcases are working and regression testing passes, then go ahead and check in/distribute this version.